[PATCH] D71516: [InstCombine] Canonicalize select immediates

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:352
+    // This is similar to ShrinkDemandedConstant, but for a select we want to
+    // try and keep the selected constants the same as icmp value constants, if
+    // we can. This helps not break apart (or helps put back together)
----------------
nit: try and keep -> try to keep


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:361
+
+      // Get the constant out of the ICmp, if there is one
+      const APInt *CmpC;
----------------
nit: add period at end of sentence.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:371-376
+      // If the constants are not already the same, but can be with the demand
+      // mask, use the constant value from the ICmp.
+      if ((*CmpC & DemandedMask) != (*SelC & DemandedMask))
+        return ShrinkDemandedConstant(I, OpNo, DemandedMask);
+      I->setOperand(OpNo, ConstantInt::get(I->getType(), *CmpC));
+      return true;
----------------
This comment/code didn't read clearly to me the first time through. It might just be me, but I'd rather see the positive comparison, followed by the default, so invert the order:
     if ((*CmpC & DemandedMask) == (*SelC & DemandedMask)) {
      I->setOperand(OpNo, ConstantInt::get(I->getType(), *CmpC));
      return true;
     }
     return ShrinkDemandedConstant(I, OpNo, DemandedMask);


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

https://reviews.llvm.org/D71516