DPDK patches and discussions
 help / color / mirror / Atom feed
From: Aaron Conole <aconole@redhat.com>
To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Cc: "dev\@dpdk.org" <dev@dpdk.org>,
	"gavin.hu\@arm.com" <gavin.hu@arm.com>,
	"konstantin.ananyev\@intel.com" <konstantin.ananyev@intel.com>
Subject: Re: [dpdk-dev] [EXT] [PATCH 1/3] acl: fix arm argument types
Date: Wed, 10 Apr 2019 11:52:38 -0400	[thread overview]
Message-ID: <f7tpnpteu2h.fsf@dhcp-25.97.bos.redhat.com> (raw)
Message-ID: <20190410155238.fennHkUPh88b6xq7kiZSQ945kBS9sPABKxgYPXiRHGI@z> (raw)
In-Reply-To: <b38d3b8e74fbf89dd40f3d9b26e436b1c30612ff.camel@marvell.com> (Jerin Jacob Kollanukkaran's message of "Wed, 10 Apr 2019 14:39:31 +0000")

Jerin Jacob Kollanukkaran <jerinj@marvell.com> writes:

> On Mon, 2019-04-08 at 14:24 -0400, Aaron Conole wrote:
>> -------------------------------------------------------------------
>> ---
>> Compiler complains of argument type mismatch, like:
>
> Can you share more details on how to reproduce this issue?

It will be generated using the meson build after enabling the neon
extension support (which isn't currently happening on ARM using meson as
the build environment).

> We already have
> CFLAGS_acl_run_neon.o += -flax-vector-conversions
> in the Makefile.
>
> If you are taking out -flax-vector-conversions the correct way to
> fix will be use vreinterpret*.
>
> For me the code looks clean, If unnecessary casting is avoided.

I agree.  I merely make explicit the casts that the compiler will be
implicitly introducing.

>
>> 
>>    ../lib/librte_acl/acl_run_neon.h: In function ‘transition4’:
>>    ../lib/librte_acl/acl_run_neon.h:115:2: note: use -flax-vector-
>> conversions
>>       to permit conversions between vectors with differing element
>> types
>>       or numbers of subparts
>>      node_type = vbicq_s32(tr_hi_lo.val[0], index_msk);
>>      ^
>>    ../lib/librte_acl/acl_run_neon.h:115:41: error: incompatible type
>> for
>>       argument 2 of ‘vbicq_s32’
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_acl/acl_run_neon.h | 46 ++++++++++++++++++++-------------
>> --
>>  1 file changed, 27 insertions(+), 19 deletions(-)
>> 
>> 
>>  
>>  /*
>> @@ -179,6 +183,9 @@ search_neon_8(const struct rte_acl_ctx *ctx,
>> const uint8_t **data,
>>  	acl_match_check_x4(0, ctx, parms, &flows, &index_array[0]);
>>  	acl_match_check_x4(4, ctx, parms, &flows, &index_array[4]);
>>  
>> +	memset(&input0, 0, sizeof(input0));
>> +	memset(&input1, 0, sizeof(input1));
>
> Why this memset only required for arm64? If it real issue, Shouldn't
> it required for x86 and ppc ?

No.  Please see the following lines (which is due to the ARM neon
intrinsic for setting individual lanes):

	while (flows.started > 0) {
		/* Gather 4 bytes of input data for each stream. */
		input0 = vsetq_lane_s32(GET_NEXT_4BYTES(parms, 0), input0, 0);
		input1 = vsetq_lane_s32(GET_NEXT_4BYTES(parms, 4), input1, 0);

Note: the first time through this loop, input0 and input1 appear on the
rhs of the assignment before appearing on the lhs.  This will generate
an uninitialized value warning, even though the assignments are to
individual lanes of the vector.

I squelched the warning from the compiler in the most brute-force way
possible.  Perhaps it would be better to use a static initialization for
the vector but this code was intended to be RFC and to generate
feedback.

I guess one alternate approach could be:

   static const int32x4_t ZERO_VEC;
   int32x4_t input0 = ZERO_VEC, input1 = ZERO_VEC;

   ...

   int32x4_t input = ZERO_VEC;

This would have the benefit of keeping the initializer as 'fast' as
possible (although I recall a memset under a certain size threshold is
the same effect, but not certain).

Either way, I prefer it to squelching the warning, since the warning
has been found to catch legitimate errors many times.

  parent reply	other threads:[~2019-04-10 15:52 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-08 18:24 [dpdk-dev] [PATCH 0/3] librte_acl: fixes related to testing with the meson build Aaron Conole
2019-04-08 18:24 ` Aaron Conole
2019-04-08 18:24 ` [dpdk-dev] [PATCH 1/3] acl: fix arm argument types Aaron Conole
2019-04-08 18:24   ` Aaron Conole
2019-04-10 14:39   ` [dpdk-dev] [EXT] " Jerin Jacob Kollanukkaran
2019-04-10 14:39     ` Jerin Jacob Kollanukkaran
2019-04-10 15:52     ` Aaron Conole [this message]
2019-04-10 15:52       ` Aaron Conole
2019-04-10 16:07       ` Jerin Jacob Kollanukkaran
2019-04-10 16:07         ` Jerin Jacob Kollanukkaran
2019-04-10 17:20         ` Aaron Conole
2019-04-10 17:20           ` Aaron Conole
2019-04-30 12:57           ` Aaron Conole
2019-04-30 12:57             ` Aaron Conole
2019-06-05 15:16     ` Jerin Jacob Kollanukkaran
2019-06-05 17:09       ` Aaron Conole
2019-04-08 18:24 ` [dpdk-dev] [PATCH 2/3] acl: update the build for multi-arch Aaron Conole
2019-04-08 18:24   ` Aaron Conole
2019-04-08 18:24 ` [dpdk-dev] [PATCH 3/3] acl: adjust the tests Aaron Conole
2019-04-08 18:24   ` Aaron Conole
2019-04-09  8:41   ` Ananyev, Konstantin
2019-04-09  8:41     ` Ananyev, Konstantin
2019-04-09 13:01     ` Aaron Conole
2019-04-09 13:01       ` Aaron Conole
2019-04-09 16:03       ` Ananyev, Konstantin
2019-04-09 16:03         ` Ananyev, Konstantin
2019-04-09 17:04         ` Ananyev, Konstantin
2019-04-09 17:04           ` Ananyev, Konstantin
2019-04-10  8:13           ` Richardson, Bruce
2019-04-10  8:13             ` Richardson, Bruce
2019-04-10 13:10           ` Aaron Conole
2019-04-10 13:10             ` Aaron Conole
2019-04-10 13:24             ` Bruce Richardson
2019-04-10 13:24               ` Bruce Richardson
2019-04-10 13:46               ` Bruce Richardson
2019-04-10 13:46                 ` Bruce Richardson
2019-04-09 17:05         ` Richardson, Bruce
2019-04-09 17:05           ` Richardson, Bruce
2019-04-09 18:29           ` Ananyev, Konstantin
2019-04-09 18:29             ` Ananyev, Konstantin
2019-04-10  9:06             ` Bruce Richardson
2019-04-10  9:06               ` Bruce Richardson
2019-04-08 20:40 ` [dpdk-dev] [PATCH 0/3] librte_acl: fixes related to testing with the meson build Aaron Conole
2019-04-08 20:40   ` Aaron Conole

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f7tpnpteu2h.fsf@dhcp-25.97.bos.redhat.com \
    --to=aconole@redhat.com \
    --cc=dev@dpdk.org \
    --cc=gavin.hu@arm.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).