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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D48893#1152643, @nlopes wrote:

> 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.


Ok, I think I get it now. Even though the shuffle mask's undef guarantees that any uses are also undef in those lanes, we're creating poison in the binops that didn't exist before. Therefore, other passes may use that to create unsound transforms. I don't think we have vector folds that are that sophisticated yet, but we can't open that door. It's similar to the problem we have with div/rem, but not as blatant. So the solution is to generalize the hack that I added for div/rem (getSafeVectorConstantForIntDivRem), so we can allow these transforms for any poison-producers. And the right way to replace undef with safe constants is almost what we have here: translate to the identity constant for a given binop when possible. Other transforms can turn that back into undef when they determine that it's safe.


https://reviews.llvm.org/D48893