[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:39
+#include "llvm/Analysis/ValueTracking.h"
+#include "llvm/IR/PatternMatch.h"
+#include "llvm/InitializePasses.h"
----------------
Not used?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:46
+using namespace llvm;
+using namespace PatternMatch;
+
----------------
not used?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:64
+class CanonicalizeFreezeInLoopsImpl {
+private:
+  Loop *L;
----------------
nit: private not needed here.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:70
+  class FrozenIndPHIInfo {
+  public:
+    FreezeInst *FI = nullptr;
----------------
public not needed here.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:84
+
+  void getIndPHIInfo(PHINode *PHI, FrozenIndPHIInfo &Info) {
+    auto *StepInst = dyn_cast<BinaryOperator>(
----------------
nit: I think it would be slightly more straight forward to have this function return  a new FrozenIndPHIInfo.


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:84
+
+  void getIndPHIInfo(PHINode *PHI, FrozenIndPHIInfo &Info) {
+    auto *StepInst = dyn_cast<BinaryOperator>(
----------------
fhahn wrote:
> nit: I think it would be slightly more straight forward to have this function return  a new FrozenIndPHIInfo.
On second look, this seems very similar to InductionDescriptor::isInductionPHI. Can we use that instead? isInductionPHI should also be able to subsume the call to isAuxiliaryInductionVariable


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:109
+// Given U = (value, user), replace value with freeze(value), and let
+// SCEV forget user. The inserted freeze is placed at the preheader.
+void CanonicalizeFreezeInLoopsImpl::InsertFreezeAndForgetFromSCEV(Use &U) {
----------------
nit: placed *in*?


================
Comment at: llvm/lib/Transforms/Utils/CanonicalizeFreezeInLoops.cpp:115
+  auto *ValueToFr = U.get();
+  assert(UserI && L->contains(UserI->getParent()));
+  if (isGuaranteedNotToBeUndefOrPoison(ValueToFr, UserI, &DT))
----------------
nit: needs message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523