Aaron Conole <acon...@redhat.com> writes:
> Jerin Jacob Kollanukkaran <jer...@marvell.com> writes:
>
>> On Wed, 2019-04-10 at 11:52 -0400, Aaron Conole wrote:
>>> Jerin Jacob Kollanukkaran <jer...@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).
>>
>>
>> Can you share the patch to enable this for testing.
>
> Sure - I'm using these:
>
> (needed)
> 1/3 - http://mails.dpdk.org/archives/dev/2019-March/128304.html
> 2/3 - http://mails.dpdk.org/archives/dev/2019-March/128305.html
>
> (following only needed for travis support)
> 3/3 - http://mails.dpdk.org/archives/dev/2019-March/128306.html
>
> -Aaron
>
>> Since the additional memcpy in fastpath, I need to check the overhead
>> and check the possibility to avoid the memcpy to case.
Were you able to test this?
>>
>>>
>>> > 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 <acon...@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.
>>
>> I will get back to this after reproducing the issue locally.
>
> Awesome - thanks.
