[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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.
----------------
'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)'


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:62
+
+/// A class that describes induction variable / frozen value / step values
+/// which is a candidate for canonicalization.
----------------
nit: A struct.



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:85
+  for (auto *BB : L->blocks()) {
+    for (auto &I : *BB) {
+      auto *FI = dyn_cast<FreezeInst>(&I);
----------------
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.



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:92
+        SE.forgetValue(FI);
+        FI->eraseFromParent();
+        continue;
----------------
There's a test case missing for that scenario. Also, I think removing FI here will invalidate your iterator over the BB, probably causing a crash. You can work around that by using make_early_inc_range(*BB) and iterating over that.

But I am not sure we actually need to do this here, as freeze without users should be cleaned up by the existing DCE, right? Not sure if we need to handle it here.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:150
+
+    LLVM_DEBUG(dbgs() << "CanonFr: PN: " << *PN << "\n"
+                      << "CanonFr:   " << *SteppingInst << "\n"
----------------
I think it would be good to be a bit more descriptive in the debug message, e.g. it could say something like 'Pushing freeze ' << %FI << 
 through ' << *SteppingInst << ' and ' << *PN << ' outside of loop' , 


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:154
+
+    if (!isGuaranteedNotToBeUndefOrPoison(SteppingInst, SteppingInst, &DT)) {
+      // Drop any flag on the stepping instruction.
----------------
Is this overly conservative? The transform ensures that both operands won't be poison, right? Would it be enough  to check if the instruction may result in poison with 2 non-poison operands?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:163
+    if (!isGuaranteedNotToBeUndefOrPoison(StepValue, PN, &DT)) {
+      unsigned OpIdx = SteppingInst->getOperand(1) == StepValue;
+      SteppingInst->setOperand(
----------------
I think currently cases where we have multiple candidates for the same PHI are not handled correctly at the moment here, as after handling the first candidate the step operand will  not match the original StepValue. Multiple candidates would be added for a loop like the one below I think.

```
loop:
  %i = phi i32 [%init, %entry], [%i.next, %loop]
  %i.fr = freeze i32 %i
  call void @call(i32 %i.fr)
  %i.next = add nsw nuw i32 %i, %step
  %i.next.fr = freeze i32 %i.next
  %cond = icmp eq i32 %i.next, %n
  call void @call(i32 %i.next.fr)
  br i1 %cond, label %loop, label %exit
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523