[PATCH] D146699: [GuardWidening] Freeze the introduced use.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:212-214
+  /// Adds freeze to Orig and push it as far as possible very aggressively.
+  /// Also replaces all uses of frozen instruction with frozen version.
+  Value *freezeAndPush(Value *Orig, Instruction *InsertPt);
----------------
Can this be a part of some Utils maybe? It looks generic enough.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:567
+
+static Instruction *getFreezeInsertPt(Value *V, Instruction *I) {
+  if (auto *I = dyn_cast<Instruction>(V))
----------------
Add comment?


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:568
+static Instruction *getFreezeInsertPt(Value *V, Instruction *I) {
+  if (auto *I = dyn_cast<Instruction>(V))
+    return I->getInsertionPointAfterDef();
----------------
use another name here, overriden `I` is confusing.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:601
+    if (any_of(I->operands(), [InsertPt](Value *Op) {
+          return isa<Instruction>(Op) && !getFreezeInsertPt(Op, InsertPt);
+        })) {
----------------
1. How can `getFreezeInsertPt` be null? Is there a test on that?
2. Is `isa<Instruction>(Op)` check really needed here? You already do this check inside of `getFreezeInsertPt`, and I guess for it it never fails.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:606
+    }
+    DropPoisonFlags.insert(I);
+    append_range(Worklist, I->operands());
----------------
Do I understand correctly that if any of these instructions is a long chain of non-overflowing arithmetics, we will just wipe flags from all of them to the very roots. hypothetically even from entire function? Isn't it an overkill?

I think we should put a limit somewhere and do not go above it. Maybe the widened guard point should be this limit.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:615
+    auto *FreezeInsertPt = getFreezeInsertPt(V, InsertPt);
+    FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr", FreezeInsertPt);
+    if (V == Orig)
----------------
Maybe add statistic to count how many freezes we add?


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:618
+      Result = FI;
+    V->replaceUsesWithIf(FI, [&](Use & U)->bool { return U.getUser() != FI; });
+  }
----------------
Erase/mark trivially dead instructions for erasing?


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

https://reviews.llvm.org/D146699