Thread: Question about the Implementation of vector32_is_highbit_set on ARM

Hi all,
I have some questions about the implementation of vector32_is_highbit_set  on arm.
Below is the comment and the implementation for this function.
/*
 * Exactly like vector8_is_highbit_set except for the input type, so it
 * looks at each byte separately.
 *
 * XXX x86 uses the same underlying type for 8-bit, 16-bit, and 32-bit
 * integer elements, but Arm does not, hence the need for a separate
 * function. We could instead adopt the behavior of Arm's vmaxvq_u32(), i.e.
 * check each 32-bit element, but that would require an additional mask
 * operation on x86.
 */
#ifndef USE_NO_SIMD
static inline bool
vector32_is_highbit_set(const Vector32 v)
{
#if defined(USE_NEON)
        return vector8_is_highbit_set((Vector8) v);
#else
        return vector8_is_highbit_set(v);
#endif
}
#endif                                                  /* ! USE_NO_SIMD */

But I still don't understand why the vmaxvq_u32 intrinsic  is not used on the arm platform.
We have used the macro USE_NEON to distinguish different platforms.
In addition, according to the "Arm Neoverse N1 Software Optimization Guide",
The vmaxvq_u32 intrinsic  has half the latency of vmaxvq_u8 and twice the bandwidth.
So I think just use vmaxvq_u32 directly.

Any comments or feedback are welcome.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you
arenot the intended recipient, please notify the sender immediately and do not disclose the contents to any other
person,use it for any purpose, or store or copy the information in any medium. Thank you. 

On Wed, Nov 8, 2023 at 2:44 PM Xiang Gao <Xiang.Gao@arm.com> wrote:
>  * function. We could instead adopt the behavior of Arm's vmaxvq_u32(), i.e.
>  * check each 32-bit element, but that would require an additional mask
>  * operation on x86.
>  */

> But I still don't understand why the vmaxvq_u32 intrinsic  is not used on the arm platform.

The current use case expects all 1's or all 0's in a 32-bit lane. If
anyone tried using it for arbitrary values, vmaxvq_u32 could give a
different answer than on x86 using _mm_movemask_epi8, so I think
that's the origin of that comment. But it's still a maintenance hazard
as is, since x86 wouldn't work for arbitrary values. It seems the path
forward is to rename this function to vector32_is_any_lane_set(), as
in the attached (untested on Arm). That would allow each
implementation to use the most efficient path, whether it's by 8- or
32-bit lanes. If we someday needed to look at only the high bits, we
would need a new function that performed the necessary masking on x86.

It's possible this method could shave cycles on Arm in some 8-bit lane
cases where we don't actually care about the high bit specifically,
since the movemask equivalent is slow on that platform, but I haven't
looked yet.

On Date: Mon, 20 Nov 2023 16:05:43PM +0700, John Naylor wrote:

>On Wed, Nov 8, 2023 at 2:44=E2=80=AFPM Xiang Gao <Xiang.Gao@arm.com> wrote:
>>  * function. We could instead adopt the behavior of Arm's vmaxvq_u32(), i=
>.e.
>>  * check each 32-bit element, but that would require an additional mask
>>  * operation on x86.
>>  */
>
>> But I still don't understand why the vmaxvq_u32 intrinsic  is not used on=
 the arm platform.

>The current use case expects all 1's or all 0's in a 32-bit lane. If
>anyone tried using it for arbitrary values, vmaxvq_u32 could give a
>different answer than on x86 using _mm_movemask_epi8, so I think
>that's the origin of that comment. But it's still a maintenance hazard
>as is, since x86 wouldn't work for arbitrary values. It seems the path
>forward is to rename this function to vector32_is_any_lane_set(), as
>in the attached (untested on Arm). That would allow each
>implementation to use the most efficient path, whether it's by 8- or
>32-bit lanes. If we someday needed to look at only the high bits, we
>would need a new function that performed the necessary masking on x86.
>
>It's possible this method could shave cycles on Arm in some 8-bit lane
>cases where we don't actually care about the high bit specifically,
>since the movemask equivalent is slow on that platform, but I haven't
>looked yet.

Thank you for your detailed explanation.
Can I do some testing and submit this patch?
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you
arenot the intended recipient, please notify the sender immediately and do not disclose the contents to any other
person,use it for any purpose, or store or copy the information in any medium. Thank you.
 

On Thu, Nov 23, 2023 at 4:29 PM Xiang Gao <Xiang.Gao@arm.com> wrote:
>
> Thank you for your detailed explanation.
> Can I do some testing and submit this patch?

Please do, thanks.