[PATCH] D44626: [InstCombine] Fold (A OR B) AND B code sequence over Phi node

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Any idea how frequently this triggers on general code (the LLVM testsuite, or something like that)?



================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:653
+  if (!Phi.getType()->isIntegerTy() ||
+      Phi.getType()->getPrimitiveSizeInBits() != 64)
+    return nullptr;
----------------
Only allowing 64-bit values seems overly restrictive.


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:657
+  Instruction *rc = nullptr;
+  for (User *User : Phi.users()) {
+    // Try to find ((%A OR %B) AND %B) code sequence over Phi.
----------------
We don't usually like to walk uses like this; can you start the pattern-match from the "and"?


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:682
+      // 32-bit, we cannot guarantee that the AND can be eliminated.
+      uint64_t Imm = dyn_cast<ConstantInt>(UserVal)->getZExtValue();
+      if ((Imm & 0xFFFFFFFFuL) != 0 && (Imm >> 32) != 0) continue;
----------------
cast<>, not dyn_cast<>


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:692
+               LowVal->getType()->getPrimitiveSizeInBits() == 32;
+      };
+      auto IsEligibleOrConcat = [&](Value *V) {
----------------
Do you need to check hasOneUse() on the "or" here?

Looking specifically for zext+shl seems overly specific to your testcase; I'd like to see something a little more general.  Maybe you could check `SimplifyAndInst(V, UserVal) != nullptr`?  Or maybe that's too expensive; not sure.


https://reviews.llvm.org/D44626