[PATCH] D144492: [InstCombine] Support multiple comparisons in foldAllocaCmp()

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:917
-  // comparisons against the alloca consistently, and avoids the risk of
-  // erroneously folding a comparison of the pointer with itself.
 
----------------
Instead of just deleting this, can you replace it with an equally useful comment about what the new logic is doing?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:925
+      // have other contributions from a select/phi operand.
+      // TODO: We could check whether getUnderlyingObjects() reduces to one
+      // object.
----------------
I don't understand this TODO, isn't `getUnderlyingObject(*U) == Alloca` doing just that?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:927
+      // object.
+      if (ICmp && ICmp->isEquality() && getUnderlyingObject(*U) == Alloca) {
+        // Collect equality icmps of the alloca, and don't treat them as
----------------
Can you add TODO to canonicalize `ule/uge/sle/sge` -> `ult/ugt/slt/sgt`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:953
+      auto *Res = ConstantInt::get(
+          ICmp->getType(), !CmpInst::isTrueWhenEqual(ICmp->getPredicate()));
+      replaceInstUsesWith(*ICmp, Res);
----------------
`!CmpInst::isTrueWhenEqual(ICmp->getPredicate())` -> `ICmp->getPredicate() == ICmpInst::ICMP_NE` is clearer imo.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:956
+      eraseInstFromFunction(*ICmp);
+      Changed = true;
+      break;
----------------
Is there a reason this isn't in `InstructionSimplify`?


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

https://reviews.llvm.org/D144492