[PATCH] D52508: [InstCombine] Clean up after IndVarSimplify

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1201
+      m_Value(Select));
+  if (match(&I, m_c_BinOp(m_Value(A), m_OneUse(m_Not(m_OneUse(SelectPred))))) &&
+      IsFreeToInvert(B, B->hasOneUse())) {
----------------
lebedev.ri wrote:
> `m_Value(A)` will *always* match.
> I wonder if would it make sense to swap them around?
> 
I believe we have to match A before the m_Deferred. I've added a test for matching both ways round.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1205-1210
+    if (cast<SelectInst>(Select)->getTrueValue() == A)
+      return SelectInst::Create(
+          Pred, ConstantInt::getAllOnesValue(A->getType()), AmC);
+    else
+      return SelectInst::Create(Pred, AmC,
+                                ConstantInt::getAllOnesValue(A->getType()));
----------------
lebedev.ri wrote:
> How about
> ```
>   Value *TrueVal = ConstantInt::getAllOnesValue(A->getType());
>   Value *FalseVal = AmC;
>   if (cast<SelectInst>(Select)->getTrueValue() != A)
>     std::swap(TrueVal, FalseVal)
>   return SelectInst::Create(Pred, TrueVal, FalseVal);
> ```
Nice.


https://reviews.llvm.org/D52508