[PATCH] D48893: [Constants, InstCombine] allow RHS (operand 1) identity constants for binops

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In https://reviews.llvm.org/D48893#1152112, @spatel wrote:

> In https://reviews.llvm.org/D48893#1151840, @nlopes wrote:
>
> > `shl %x, undef` is poison; we don't want more undefs :)
> >  So this transformation for shifts is not correct. The way to make it correct would be to introduce a `poison` value and use it in the shuffle instructions instead of undef. I suggest we finally go ahead and do that.
>
>
>
>
> 1. Does the 'undef' in the shuffle mask represent something different than the 'undef' in the shift amount?


No: undef means any value in both cases.
The deal is that x << y is poison if y >= bitwidth. Since undef can be such a number, x << undef is poison.
Why is it poison? Because different CPU architectures (e.g. x86 and ARM) deal with this case differently, so we don't really want to define it. C/C++ allows to do that.

Shuffle is never poison if its inputs are not poison.

> 2. Are we distinguishing shifts from the other binary opcodes here? Or is this existing transform also incorrect?
> 
>   ``` %b = mul nsw nuw <4 x i32> %v, <i32 11, i32 12, i32 13, i32 14> %s = shufflevector <4 x i32> %v, <4 x i32> %b, <4 x i32> <i32 undef, i32 5, i32 2, i32 7> --> %s = mul nuw nsw <4 x i32> [[V:%.*]], <i32 undef, i32 12, i32 1, i32 14> ```

This transformation is also incorrect, unfortunately. If you drop nsw & nuw, it's correct.


https://reviews.llvm.org/D48893