[PATCH] D75859: [CodeGenPrepare] Fold br(freeze(icmp x, const)) to br(icmp(freeze x, const))

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I agree that this can and should be generalized elsewhere in the optimizer, but having this local peephole in CGP is probably a reasonable starting point.  The general framing would be something like "move freeze towards definition of possible poison".  We could do so not just for cmps, but for any instruction according to the poison propagation rules and the may be poison analysis results.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7197
+    // This helps generate efficient conditional jumps.
+    bool AllUsesAreBr = llvm::all_of(
+        FI->users(), [](const User *U) { return isa<BranchInst>(U); });
----------------
I don't think you actually need the branch check here.  Unless I'm missing something, moving freeze into both operands is legal, and since one of them is obviously not poison, we can only move it into one of the two.  I don't think it matters which uses we have.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7199
+        FI->users(), [](const User *U) { return isa<BranchInst>(U); });
+    auto II = dyn_cast<ICmpInst>(FI->getOperand(0));
+    if (AllUsesAreBr && II && II->hasOneUse()) {
----------------
Please structure as:

if (FreezeInst *FI = ...)
  if (ICmpInst *ICI = ...)
   if (other checks...)


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7207
+        if (!Const0 || !Const1) {
+          Instruction *F = new FreezeInst(Const0 ? Op1 : Op0, "fr", II);
+          II->setOperand(Const0 ? 1 : 0, F);
----------------
Take the name of the original freeze?


================
Comment at: llvm/test/Transforms/CodeGenPrepare/X86/freeze-icmp.ll:6
+
+define void @f(i32 %a) {
+; CHECK-LABEL: @f(
----------------
You need a test for the swapped operand, non-constant arg (i.e. nop), and both constants cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75859