[PATCH] D47686: [InstCombine] refine UB-handling in shuffle-binop transform

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

This makes sense to me, but i'm not really familiar with `shufflevector`,
so i'm going to leave the actual LGTM to others.



================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1420
     if (MayChange) {
-      // It's not safe to use a vector with undef elements because the entire
-      // instruction can be folded to undef (for example, div/rem divisors).
-      // Replace undef lanes with the first non-undef element. Vector demanded
-      // elements can change those back to undef values if that is safe.
-      Constant *SafeDummyConstant = nullptr;
-      for (unsigned i = 0; i < VWidth; ++i) {
-        if (!isa<UndefValue>(NewVecC[i])) {
-          SafeDummyConstant = NewVecC[i];
-          break;
-        }
-      }
-      assert(SafeDummyConstant && "Undef constant vector was not simplified?");
-      for (unsigned i = 0; i < VWidth; ++i)
-        if (isa<UndefValue>(NewVecC[i]))
-          NewVecC[i] = SafeDummyConstant;
+      // With div/rem instructions, it is not safe to use a vector with undef
+      // elements because the entire instruction can be folded to undef.
----------------
Then i'd suggest
```
// With integer div/rem instructions, it is not safe to use a vector with undef
```
but that will require reflowing the entire comment :S


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1431
+          if (isa<UndefValue>(NewVecC[i]))
+            NewVecC[i] = ConstantInt::get(Inst.getType()->getScalarType(), 1);
 
----------------
spatel wrote:
> lebedev.ri wrote:
> > We want `int(1)` even for `float`s?
> We're only changing integer opcodes/operands here. I can assert that if it would make the code clearer?
I think this is precisely the case of things to assert, so ideally yes please.


https://reviews.llvm.org/D47686