[Clang][X86] Change X86 cast intrinsics to use __builtin_nondeterministic_value

[ManuelJBrito]

Summary

The following intrinsics are currently implemented using a shufflevector with an undefined mask, this is however incorrect according to intel's semantics for undefined value which expect an unknown but consistent value.

With __builtin_nondeterministic_value we can now match intel's undefined value.

Related patch for more context : https://reviews.llvm.org/D103874

[craig.topper]

We have a couple bugs that show (freeze (poison)) doesn't work past SelectionDAG. Is that a concern here? The most recent https://github.com/llvm/llvm-project/issues/60429

[RKSimon]

What do we gain from using __builtin_nondeterministic_value instead of just setzero? https://godbolt.org/z/zrb6858Mr

[ManuelJBrito]

In D143287#4103597, @craig.topper wrote:

We have a couple bugs that show (freeze (poison)) doesn't work past SelectionDAG. Is that a concern here? The most recent https://github.com/llvm/llvm-project/issues/60429

I don't think it's a concern . Here https://godbolt.org/z/1ecM8roYh the lowering seems ok.

[ManuelJBrito]

In D143287#4104056, @RKSimon wrote:

What do we gain from using __builtin_nondeterministic_value instead of just setzero? https://godbolt.org/z/zrb6858Mr

__builtin_nondeterministic_value is lowered to freeze(poison)

These intrinsics are meant to be no-op and we can't to that with zeroinitializer, although as of right now instcombine seems to transform the freeze(poison) into zeroinitializer(as shown by your example), so i'll make another patch to fix that.

[craig.topper]

In D143287#4107439, @ManuelJBrito wrote:

In D143287#4103597, @craig.topper wrote:

We have a couple bugs that show (freeze (poison)) doesn't work past SelectionDAG. Is that a concern here? The most recent https://github.com/llvm/llvm-project/issues/60429

I don't think it's a concern . Here https://godbolt.org/z/1ecM8roYh the lowering seems ok.

The bugs start occurring if you use the same nondeterministic value multiple times and expect the value to be the same for all uses.

[ManuelJBrito]

In D143287#4108165, @craig.topper wrote:

In D143287#4107439, @ManuelJBrito wrote:

In D143287#4103597, @craig.topper wrote:

We have a couple bugs that show (freeze (poison)) doesn't work past SelectionDAG. Is that a concern here? The most recent https://github.com/llvm/llvm-project/issues/60429

I don't think it's a concern . Here https://godbolt.org/z/1ecM8roYh the lowering seems ok.

The bugs start occurring if you use the same nondeterministic value multiple times and expect the value to be the same for all uses.

I understand now ... so if the bug is present it works as if it were an undef. For these intrinsics we would just regress to the current behavior.
So is it okay if we land this patch? Because it will still be an improvement.

[ManuelJBrito]

Add end to end tests

Currrently these expect a mov that will be removed when the instcombine bug is fixed

[RKSimon]

clang/test/CodeGen/X86/avx-builtins.c

146–147

Would it be useful to add a check for a "freeze <2 x double> poison" (or similar) to each cast test?

[ManuelJBrito]

Match freeze(poison) operand in tests

[ManuelJBrito]

Update tests after D143593.

There are some performance regressions with casts from 128 to 512. The backend inserts vinsertf instructions. So that has to be fixed.
In D130339 @RKSimon mentioned something about custom vector widening patterns that need to be adjusted to handle freeze(undef), any pointers for how a patch for that would look like?

[RKSimon]

In D143287#4150186, @ManuelJBrito wrote:

There are some performance regressions with casts from 128 to 512. The backend inserts vinsertf instructions. So that has to be fixed.
In D130339 @RKSimon mentioned something about custom vector widening patterns that need to be adjusted to handle freeze(undef), any pointers for how a patch for that would look like?

IIRC there are various places in X86ISelLowering where we try to match vector widening/concat patterns - such as collectConcatOps/getTargetShuffleAndZeroables/resolveTargetShuffleInputsAndMask (and many more) - it should be fine to address these on a case by case basis though, as many should just disappear via SimplifyDemandedVectorElts etc.

[aqjune]

H, is D104790 superseded by this patch? I wonder what is the status of this patch as well.

[ManuelJBrito]

In D143287#4179020, @aqjune wrote:

H, is D104790 superseded by this patch?

I don't think so we still need to fix the undefined intrinsics, right? Maybe I'm not understanding the question.

I wonder what is the status of this patch as well.

We need to land D144903 first to have the correct assembly, but it's currently held up by a crash in one of the tests due to an unrelated issue.

[aqjune]

In D143287#4179344, @ManuelJBrito wrote:

In D143287#4179020, @aqjune wrote:

H, is D104790 superseded by this patch?

I don't think so we still need to fix the undefined intrinsics, right? Maybe I'm not understanding the question.

Oh right, D104790 and this deal with different intrinsics, thanks.
I was wondering whether I could clean up some of my open patches.

I wonder what is the status of this patch as well.

We need to land D144903 first to have the correct assembly, but it's currently held up by a crash in one of the tests due to an unrelated issue.

I see, hope that the crash is fixed soon..!

[ManuelJBrito]

Rebase

avx-cast-builtins.c was moved to D144903

[ManuelJBrito]

Implementing the 128 to 512 casts by filling the rest of the vector with the same definition of a nondeterministic_value is not correct because :

a = freeze poison
v = <a, a>

is not the same as

v = freeze poison

The only solution I'm seeing ,using the shufflevector, is doing the conversion in two steps:

• build a 256 vector with the upper half being undefined( freeze poison)
• build a 512 vector where the lower half is the previous 256 vector and the upper half being undefined

I think this would require two shuffles which is unfortunate.

This would ensure no miscompilations due to multiple uses of the same freeze undef/poison but would probably require some backend work to ensure the pattern is recognized to emit efficient assembly.

Would this work ? @RKSimon @craig.topper

[nlopes]

In D143287#4202174, @ManuelJBrito wrote:

Implementing the 128 to 512 casts by filling the rest of the vector with the same definition of a nondeterministic_value is not correct because :

a = freeze poison
v = <a, a>

is not the same as

v = freeze poison

The only solution I'm seeing ,using the shufflevector, is doing the conversion in two steps:

• build a 256 vector with the upper half being undefined( freeze poison)
• build a 512 vector where the lower half is the previous 256 vector and the upper half being undefined

I think this would require two shuffles which is unfortunate.

This would ensure no miscompilations due to multiple uses of the same freeze undef/poison but would probably require some backend work to ensure the pattern is recognized to emit efficient assembly.

Semantically that is correct.
But the backend may require some tweaks to recognize this pattern.

[ManuelJBrito]

Update to remove multiple uses of freeze poison.
I am unsure about the code style used in the 128 to 512 casts. Any comments are appreciated.

[nlopes]

LGTM, but please wait for another reviewer.

[ManuelJBrito]

Any further comments @RKSimon ?

[RKSimon]

LGTM - just simplify the shuffle masks (even if it break 80-col).

Please keep an eye out for any regressions, I'm not certain we've shaken out every possible issue.

clang/lib/Headers/avx512fintrin.h

401

We don't always keep to clang formatting in the headers if it confuses things - better to keep the entire shuffle mask on a single line if possible - same for the others

return __builtin_shufflevector(__a, __builtin_nondeterministic_value(__a),
                               0, 1, 2, 3, 4, 5, 6, 7);

409

Maybe:

return __builtin_shufflevector(__a, __builtin_nondeterministic_value(__a), 0,
                               0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15);

clang/test/CodeGen/X86/avx-cast-builtins.c

102

fixme

[ManuelJBrito]

Thanks for the review! I'll simplify the masks.