[PATCH] D48987: [InstCombine] drop poison flags for shuffle transforms with undefs

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: test/Transforms/InstCombine/shuffle_select.ll:30-42
 ; If any element of the shuffle mask operand is undef, that element of the result is undef.
 ; The shuffle is eliminated in this transform, but we can replace a constant element with undef.
-; Preserve flags when possible.
+; Preserve flags when possible. It's not safe to propagate poison-generating flags with undef constants.
 
 define <4 x i32> @mul(<4 x i32> %v) {
 ; CHECK-LABEL: @mul(
+; CHECK-NEXT:    [[S:%.*]] = mul <4 x i32> [[V:%.*]], <i32 undef, i32 12, i32 1, i32 14>
----------------
lebedev.ri wrote:
> >>! In D48987#1153586, @spatel wrote:
> >>>! In D48987#1153565, @lebedev.ri wrote:
> >> So my concerns weren't unfounded :/
> >> Do we want to also do such kind of fixup in `VisitShl()` in instcombine, for every `shl` we visit?
> > 
> > Do you have an example of the problem you're thinking of? AFAIK, we won't hit this problem with scalars or general transforms because they don't modify constants like we're doing here. We do have a related foldShuffledBinop() function that I will review/fix if this patch looks right.
> 
> https://godbolt.org/g/y7hCki
> 
> Should instcombine be dropping these flags just because?
> Or we only care about not creating more of these situations than we already had?
No, we don't want to drop flags on existing instructions. Presumably, whoever created that instruction did so correctly, so there actually is poison in the vector lane with undef. 

We can't create poison where there wasn't any to start with, but we also don't want to remove poison when it exists without good reason. At least that's my understanding...happy to be corrected by people closer to poison/undef than me. :)


https://reviews.llvm.org/D48987