[PATCH] D144760: [InstCombine] use demanded vector elements to eliminate partially redundant instructions

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1715
   BinaryOperator *BO;
   if (match(I, m_BinOp(BO)) && !BO->isIntDivRem() && !BO->isShift()) {
+    Value *X = BO->getOperand(0);
----------------
goldstein.w.n wrote:
> Why doesn't this work for div/rem/shift?
**This** patch should be fine with any binop since we're not altering any instructions, but I left it inside this block as a first step to try to be safer. I can put a TODO comment on it.

The existing code is definitely not safe for div/rem because it can do this:
  %u = udiv <2 x i32> %x, <i32 42, i32 1>
  %s = shufflevector <2 x i32> %u, <2 x i32> poison, <2 x i32> zeroinitializer
  -->
  %u = udiv <2 x i32> %x, <i32 42, i32 poison>  ; we don't demand element 1, so replace constant 1
  %s = shufflevector <2 x i32> %u, <2 x i32> poison, <2 x i32> zeroinitializer

...but now we have a divide-by-poison element which can be replaced by a divide-by-0 element which means the whole instruction is UB/poison, and that's wrong of course.

I need to look through the old patches to see if there's still potential to do bad things with shifts.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144760/new/

https://reviews.llvm.org/D144760