[libclc][hip] Fix half shuffles and reenable reduction test

[JackAKirk]

• Fix broken half shuffles on amd.
• Reenable Reduction test.

Fix is to bitcast to the storage type of half (unsigned short) without doing a type conversion, before then extending to int for the shuffle.

[jchlanda]

This is a nit, but I'd go with unsigned int, even thought we're type punning back to half.

[JackAKirk]

ta done

[steffenlarsen]

Beware that this relies on UB. See https://en.cppreference.com/w/cpp/language/union:

It is undefined behavior to read from the member of the union that wasn't most recently written.

That said, I don't know of a better way to get it done... Unless we can use some sort of bitcast builtin here. Have you had a look at alternatives?

[JackAKirk]

Yeah it is UB according to openclc for one half of the 32 bits (which one depends on endianness).
If openclc inherited memcpy from c99 it would be possible to do it that way cleanly, but it doesn't. It has a section that discusses the issue briefly: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#reinterpreting-data-as-another-type
One alternative that is not UB is to first do a conversion to float, then do as_int(float), and do the reverse after the shuffle. I tried this and compared the result with doing it the way I have here to check that it gives the same result. However doing it that way generates the extra machine instructions to do the float converts etc.
HIP fp16 headers is open source and I can see that they use the union trick. Hence I figured that if they do it we may as well do it. But at the same time I could just do it via the float route.

[JackAKirk]

And yeah and I looked at amd clang builtins and I don't think they have a bitcast one (actually cuda doesn't either). For cuda I used inline ptx asm, but for amd this is more dangerous because they don't have an equivalent to ptx that is arch generation/family independent.

[steffenlarsen]

Ah, fair point. I missed the fact that this is OpenCL C. I think it is still better to not rely on UB, unless the performance overhead is high. The compiler moves fast and it wouldn't be the first time UB suddenly becomes an issue.

[JackAKirk]

It makes the add test cases in the reduction test I reenabled in this PR ~10% slower if I go via the float route. I think it is fine to do it like that if you want? I guess that fp16 shuffles are not so likely to be performance critical for much stuff, although I can't say that with confidence.

[steffenlarsen]

Ah, I didn't realize you meant cast the pointer to unsigned short *. I still think this is an alias violation, so the compiler could easily go do some magic that gives us the wrong behavior.

If half has a unsigned int storage type behind the scenes, could we maybe use some inline AMD instruction helpers here to do the conversion? It might even be as simple as a no-op or ID operation, assuming the representation is the same in the resulting AMD representation.

[JackAKirk]

Ah, I didn't realize you meant cast the pointer to unsigned short *. I still think this is an alias violation, so the compiler could easily go do some magic that gives us the wrong behavior.

I see thanks.

If half has a unsigned int storage type behind the scenes, could we maybe use some inline AMD instruction helpers here to do the conversion? It might even be as simple as a no-op or ID operation, assuming the representation is the same in the resulting AMD representation.

Generally I think inline AMD should be avoided since it is kind of like having inline assembly for SASS if it were cuda. For amd I think clang builtins should always be used that are lowered to the appropriate AMD asm.
I've found a non UB way of doing it anyway via as_ushort. I had tried this before, but with using as_short etc, which didn't work, but the as_ushort etc works perfectly.

[JackAKirk]

I've found a non UB way of doing it anyway via as_ushort. I had tried this before, but with using as_short etc, which didn't work, but the as_ushort etc works perfectly.

Actually as_short works too, I must have just messed it up the first time.

[steffenlarsen]

Great, thank you! I like this solution. 😄

[ldrumm]

FYI Type punning through a union is not UB in C99 but this is moot now this code uses as_uchar

The original C standard missed some wording that should have come from c89, and they corrected it in the early 2000s:

https://open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm

OpenCL C seems to have missed this, and also declared type punning through a union as the ordained way to do this as per https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#reinterpreting-types-using-unions

[steffenlarsen]

💯