[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM with some optional nits and a few comments about the tests. Please wait with committing for a day or so incase there are any additional comments.



================
Comment at: llvm/lib/Passes/PassBuilder.cpp:182
 #include "llvm/Transforms/Utils/CanonicalizeAliases.h"
+#include "llvm/Transforms/Utils/CanonicalizeFreezeInLoops.h"
 #include "llvm/Transforms/Utils/EntryExitInstrumenter.h"
----------------
nit: unused?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:66
+
+  struct FrozenIndPHIInfo {
+    FreezeInst *FI = nullptr;
----------------
nit: It would be good to add comments to the members here, especially how they are related.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:73
+
+  bool canHandleInst(const Instruction *I) {
+    auto Opc = I->getOpcode();
----------------
nit: It would be good to clarify what 'can handle' means here in function comment. Potentially move the comment about dropping nsw/nuw flags into it/


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:95
+
+  auto *UserI = dyn_cast<Instruction>(U.getUser());
+  auto *ValueToFr = U.get();
----------------
nit: you can use cast instead of dyn_cast. cast asserts that the value can be cast and you can UserI from the asset below.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:126
+    FrozenIndPHIInfo Info;
+    Info.PHI = &PHI;
+    Info.StepInst = ID.getInductionBinOp();
----------------
nit: consider initializing PHI & StepInst via constructor.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:145
+    for (auto *U : PHI.users()) {
+      if (auto *FI = dyn_cast<FreezeInst>(U)) {
+        LLVM_DEBUG(dbgs() << "canonfr: found from PHI's use: " << *FI << "\n");
----------------
nit: This does exactly the same as for the users of Info.StepInst, right (modulo the debug message). Might be good to use something like concat_range or a lambda for the common body.


================
Comment at: llvm/test/Transforms/CanonicalizeFreezeInLoops/func_from_mcf_r.ll:3
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
same about AArch64 triple as for llvm/test/Transforms/CanonicalizeFreezeInLoops/onephi.ll.

Also, it would be good to reduce the test to the important bits. For example, the types <{ i64, %struct.arc*, %struct.g }>, <{ i64, %struct.arc*, %struct.g } and the global aliases are quite verbose and do not really impact the freeze hoisting. 


================
Comment at: llvm/test/Transforms/CanonicalizeFreezeInLoops/onephi.ll:4
+; A set of tests that have one phi node
+target triple = "aarch64-unknown-linux-gnu"
+declare void @call(i32)
----------------
does this require an AArch64 triple? If so, you have to add something like `REQUIRES: aarch64-registered-target` I think, otherwise it might if LLVM is built without AARch64 backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523