[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked 19 inline comments as done.
aqjune added a comment.

Marked reflected/old comments as done



================
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) &&
----------------
jdoerfert wrote:
> 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.
The assertion has been removed


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:10
+// This pass canonicalizes freeze instructions in a loop.
+// Freeze instructions that use induction variables are pushed out of the loop.
+// This helps scalar evolution not be blocked by frozen variables.
----------------
fhahn wrote:
> 'use induction variables' is a bit imprecise IMO; there are no induction variables in IR. You could say 'freeze instructions that use either an induction PHI or the corresponding 'step' instruction (= the incoming value from the loop latch of the induction PHI)'
replaced it with 'induction PHI' in all places.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:85
+  for (auto *BB : L->blocks()) {
+    for (auto &I : *BB) {
+      auto *FI = dyn_cast<FreezeInst>(&I);
----------------
fhahn wrote:
> An alternative approach would be to look at the phis of the loop header and use InductionDescriptor::isInductionPHI to identify induction PHIs and their connected 'step' instruction. Then look at their users to see if they contain a freeze instruction. Candidates would be {PHI, IVDescripter, [list of freeze users to eliminate]}.
> 
> I think this potentially could simplify the code a bit. The alternative has a slightly different compile-time cost: with the alternative approach, you always have to pay to cost to find the induction PHIs (shouldn't be too costly, as the information is available in SE already), with the current approach you always have to iterate over all instructions in the loop. But I think that's unlikely to really matter in the grand scheme of things. The current approach potentially leads to duplicated induction checks currently.
> 
> The alternative approach would also handle the case where we have multiple freeze instructions using the same induction PHIs/steps without having multiple candidates, meaning each induction PHI/step would be frozen exactly once, not multiple times as with the current approach.
> 
Now the code iterates over phis first, as you suggested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523