[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I might have missed this but what was the reason we don't do this as part of LICM?



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:116
+      assert(SteppingInst && "Increment operation isn't found!");
+      assert((SteppingInst->getOpcode() == Instruction::Add ||
+              SteppingInst->getOpcode() == Instruction::Sub) &&
----------------
aqjune wrote:
> efriedma wrote:
> > What guarantees that SteppingInst is an add or sub?
> It is `L->isAuxiliaryInductionVariable`.
I have a bad feeling about this but if people think it's ok I'm fine with an assert.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:71
+  // Maintains (freeze inst, ind var, stepping instruction, step value)
+  SmallVector<std::tuple<FreezeInst *, PHINode *, BinaryOperator *, Value *>, 4>
+      Candidates;
----------------
Is there a good reason not to make this a struct with named members so the values and use sites have meaning?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:74
+
+  for (auto BB : L->blocks()) {
+    for (auto &I : *BB) {
----------------
`auto *BB` if these are pointers.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:99
+      } else if ((SteppingInst = dyn_cast<BinaryOperator>(FI->getOperand(0)))) {
+        auto IsAuxiliaryIndVar = [&](unsigned OpIdx, PHINode *&PN) -> bool {
+          PHINode *I = dyn_cast<PHINode>(SteppingInst->getOperand(0));
----------------
efriedma wrote:
> OpIdx is unused?
You shadow PN even though you capture everything. Choose a different name, return the PHI, or use the captured version of PN?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:114
+
+      assert(PN && "Induction variable isn't found!");
+      assert(SteppingInst && "Increment operation isn't found!");
----------------
Please initialize PN with null for the future, similar SteppingInst.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:129
+        }
+      }
+      Candidates.emplace_back(FI, PN, SteppingInst, Step);
----------------
Shouldn't we teach SCEV about freeze instead?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:142
+    LLVM_DEBUG(dbgs() << "CanonFr:   " << *SteppingInst << '\n');
+    LLVM_DEBUG(dbgs() << "CanonFr:   " << *FI << '\n');
+
----------------
Style: Single DEBUG? If you really want multiple dbgs() make it `LLVM_DEBUG({ ... });`


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:171
+      SE.forgetValue(PN);
+    }
+
----------------
Do we handle the case where both are `GuaranteedNotToBeUndefOrPoison` somewhere else? In that case we don't need to drop the poison generating flags, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523