Re: [PATCH] SVE popcount support

On Thu, Mar 27, 2025 at 03:31:27PM +0700, John Naylor wrote:
> On Thu, Mar 27, 2025 at 10:38 AM Nathan Bossart
> <nathandbossart(at)gmail(dot)com> wrote:
>> I also noticed a silly mistake in 0003 that would cause us to potentially
>> skip part of the tail. That should be fixed now.
>
> I'm not sure whether that meant it could return the wrong answer, or
> just make more work for paths further down.
> If the former, then our test coverage is not adequate.

This one is my bad. I think the issue is that I'm writing this stuff on a
machine that doesn't have SVE, so obviously my tests are happy as long as
the Neon stuff is okay. We do have some tests in bit.sql that should in
theory find this stuff. I'll be sure to verify all of this on a machine
with SVE...

> Aside from that, I only found one more thing that may be important: I
> tried copying the configure/meson checks into godbolt.org, and both
> gcc and clang don't like it, so unless there is something weird about
> their setup (or my use of it) it's possible some other hosts won't
> like it either.:
>
> ```
> <source>:29:10: error: call to 'svwhilelt_b8' is ambiguous
> pred = svwhilelt_b8(0, sizeof(buf));
> ^~~~~~~~~~~~
> /opt/compiler-explorer/clang-16.0.0/lib/clang/16/include/arm_sve.h:15526:10:
> note: candidate function
> svbool_t svwhilelt_b8(uint64_t, uint64_t);
> ^
> /opt/compiler-explorer/clang-16.0.0/lib/clang/16/include/arm_sve.h:15534:10:
> note: candidate function
> svbool_t svwhilelt_b8(int32_t, int32_t);
> ^
>
> <source>: In function 'autoconf_popcount_test':
> <source>:29:24: error: call to 'svwhilelt_b8' is ambiguous; argument 1
> has type 'int32_t' but argument 2 has type 'uint64_t'
> 29 | pred = svwhilelt_b8(0, sizeof(buf));
> | ^~~~~~~~~~~~
> Compiler returned: 1
> ```
>
> ...Changing it to
>
> pred = svwhilelt_b8((uint64_t)0, sizeof(buf));"
>
> clears it up.

Huh, this makes sense, but for some reason Apple clang is fine with it. In
any case, I see that we can optionally specify the expected types of the
arguments by using svwhilelt_b8_u32() (or _u64, etc.). IMHO that is far
clearer. I'm going to add that to all intrinsics that support it in the
next version of the patch set.

--
nathan