[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I'm asking myself if it wouldn't be better to start with the freeze instructions and work our way up through the operands. At the end of the day we do a lot of work just to figure out there is no freeze or the instruction chains we build do not end in one. Am I missing something?

Also, could this drop flags of instructions not on the path between a phi and a freeze? So do we keep the `nsw` on the sub and mul in the following example:

  %iv = phi [0, ...] [%step, ...]
  %step = add nsw %iv, 1
  %f = freeze %step
  %not_step1 = sub nsw %iv, 1
  %not_step2 = mul nsw %step, 2



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:77
+           Opc == Instruction::Mul;
+  }
+
----------------
Nit: I would call this `canHandleInst` or something similar. We can deal with various instructions I guess, some require us to drop flags but others not, e.g., freeze or bitcast.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:90
+    return {StepInst, StepInst->getOperand(0) == PHI};
+  }
+
----------------
What happens if the step value looks like this:

```
%iv = phi [0, ...] [%step, ...]
%step = add %iv, %iv
```
Just checking that we don't have a problem in this case.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:122
+    unsigned CurDepth) {
+  for (auto U = I->user_begin(), E = I->user_end(); U != E; ++U) {
+    Instruction *I2 = dyn_cast<Instruction>(*U);
----------------
Style: `for (const auto &U : I->users())` ? (w/ or w/o const)


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:137
+    }
+    Visited[I2].insert(I);
+
----------------
Is the find stuff necessary or could you just check the return value of the `insert` call?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:140
+    if (CurDepth < MaxDepth)
+      getReachableInstsInsideLoop(I2, Visited, CurDepth + 1);
+  }
----------------
I think we often avoid such recursion in favor of worklists but if this doesn't show on the profile it's OK I guess.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:150
+  // so only visit instructions that are known how to be dealt with
+  getReachableInstsInsideLoop(&PHI, Insts, true);
+}
----------------
`true` is an unsigned for the callee, maybe `/* CurDepth */ 1` instead?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:165
+  SmallSet<Instruction *, 8> Visited;
+  std::queue<Instruction *> Queue;
+  Queue.push(FI);
----------------
Do we really need a queue or would LIFO (=smallvector) also work?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:281
+  SmallVector<FreezeInst *, 4> FIsToRemove;
+  SmallVector<Use *, 4> UsesToInsertFr;
+
----------------
Nit: I guess we could make all these class members, right? Unclear if that is better though.

Nit: DenseMap instead of std::map?

^ Both should be considered but can be kept this way too.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:302
+        continue;
+    }
+
----------------
Nit: StepI is checked twice for null.
Style: I'd use the braces around the inner conditional instead


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:312
+  // pushed out of the loop.
+  for (auto Pair : InstsFromPHIs) {
+    auto *FI = dyn_cast<FreezeInst>(Pair.first);
----------------
`const auto &Pair`

I think `const` and `*` or `&` should be combined with `auto` whenever possible.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523