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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n 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);
----------------
spatel wrote:
> 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.
> **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.

Okay, add TODO then patch lgtm.


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

https://reviews.llvm.org/D144760