Wrong type in `__count_leading_zeros_varying_i64` in `generic.ispc`

[zephyr111]

While implementing a new feature, I just found out that the function __count_leading_zeros_varying_i64 in generic.ispc internally store the result into a 32-bit integer variable while the function returns a 64-bit type. Do you agree? If so, should the function return a 32-bit int or the result be a 64-bit one? Considering the function in the doc, I would say that this is the former (32-bit return type).

The same thing likely also applies on __count_trailing_zeros_varying_i64 and the uniform versions (__count_trailing_zeros_uniform_i64 and __count_leading_zeros_uniform_i64). What about using an explicit cast like in __popcnt_xxx?

Besides, in the documentation, the output type is not clear for 64-bit types since it is not explicitly reported:

A few functions determine how many leading bits in the given value are zero and how many of the trailing bits are zero;
there are also unsigned variants of these functions and variants that take int64 and unsigned int64 types.

int32 count_leading_zeros(int32 v)
uniform int32 count_leading_zeros(uniform int32 v)
int32 count_trailing_zeros(int32 v)
uniform int32 count_trailing_zeros(uniform int32 v)

[azwolski]

I would opt for returning a 32-bit integer type and cast it like __popcnt_xxx when needed.

[dbabokin]

The returned value is in range of [0, 64], so using 64 bits to represent that is wrong. Switch to 32 bits sounds like the right thing to do. Smaller types are probably also not a good choice as it would trigger the extension to 32 bits anyway once the value received.

[elena901]

If so, should the function return a 32-bit int or the result be a 64-bit one? What about using an explicit cast like in __popcnt_xxx?

May it cause inefficiency mentioned in #3615 ?

Madhav Varshney:

(Also, while looking at the compiler explorer output, I noticed some other inefficiencies in the codegen that I can follow up with. For example, stdlib's popcnt(varying int64) returns a varying int rather than varying int64, which adds extra instructions to convert from 4x64 down to 4x32 when that may not have been necessary. Would need to test if casting the return value back to a wider width is sufficient for that to be optimized out.)

And my proposal still we need to reopen this issue for general purposes.

[elena901]

And my proposal still we need to reopen this issue for general purposes.

[elena901]

May it cause inefficiency mentioned in #3615 ?

bump.

[aneshlya]

@elena901, the narrowing already exists in __count_leading_zeros_varying_i64 today - the body stores @llvm.ctlz into a varying int32 local and implicitly extends it back to i64. Returning int32 with an explicit cast doesn't add any conversions; it just makes the declared type match what the code is already doing, and aligns ctlz/cttz with popcnt.

Result is in [0, 64], so int32 is the right public type and matches what the docs already say for the 32-bit overloads.

I don't understand what issue you propose to reopen, popcnt for avx2 issue is tracked in #3676.

[elena901]

an explicit cast doesn't add any conversions

Probably. Same for popcnt? (I will check the code later). So why this inefficiency was mentioned? It's one of the reasons why i propose to reopen #3615 .

popcnt for avx2 issue is tracked in #3676.

It's a very nice issue, but it wasn't created by Madhav Varshney 🙂 . As i remember, it was created by one of the maintainers (probably you).

[aneshlya]

but it wasn't created by Madhav Varshney

It doesn't matter who created it. It's continuation of #3615 and it has links to it. One issue is one piece of work (ideally one PR).

[elena901]

Probably, but i'm too shy to tag @madhavarshney there 🙂 .

an explicit cast doesn't add any conversions

Probably. Same for popcnt? (I will check the code later). So why this inefficiency was mentioned? It's one of the reasons why i propose to reopen #3615 .

popcnt for avx2 issue is tracked in #3676.

It's a very nice issue, but it wasn't created by Madhav Varshney 🙂 . As i remember, it was created by one of the maintainers (probably you).

UPD.