[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Juneyoung Lee via Phabricator via llvm-commits]

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

For easier review, I agree with @fhahn .
I'll split this patch so first it only deals with a simple case (consider the case when freeze uses either phi or step instruction only). This will make the patch smaller, and also compilation time concern can be addressed as well.
I'll check the performance impact, and if it needs more general version to fully address the slowdown, it will be the following one.



================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:90
+    return {StepInst, StepInst->getOperand(0) == PHI};
+  }
+
----------------
jdoerfert wrote:
> 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.
It will be safe (won't be optimized). The test step_inst at onephi.ll checks it.


================
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);
+}
----------------
jdoerfert wrote:
> `true` is an unsigned for the callee, maybe `/* CurDepth */ 1` instead?
Giving true wasn't intended, I removed the value.


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

Previously there was one candidate object per phi, so making it as a class with named fields was making sense (instead of having tuple).
Here, I think leaving it as variables is also okay, maybe.

> Nit: DenseMap instead of std::map?
Just found that LLVM coding convention also suggests using ADTs, so fixed


================
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);
----------------
jdoerfert wrote:
> `const auto &Pair`
> 
> I think `const` and `*` or `&` should be combined with `auto` whenever possible.
> 
Oops sorry, fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523