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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

Will update patch on Monday.



================
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);
----------------
mkazantsev wrote:
> Can this be a part of some Utils maybe? It looks generic enough.
If there will be other users we can add this to utils.
At this moment it is similar what InstCombine does but a lot more aggressively.



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


================
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();
----------------
mkazantsev wrote:
> use another name here, overriden `I` is confusing.
agreed


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:601
+    if (any_of(I->operands(), [InsertPt](Value *Op) {
+          return isa<Instruction>(Op) && !getFreezeInsertPt(Op, InsertPt);
+        })) {
----------------
mkazantsev wrote:
> 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.
If Op is a result of invoke, and there is a critical edge, you have no place to put freeze.
In function getFreezeInsertPt, getInsertionPointAfterDef() may return false.

I will add a test.

Here. isa<Instruction>(Op) is just a quick check to avoid computation of first instruction in a function it is not an instruction. But we can safely remove this check.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:606
+    }
+    DropPoisonFlags.insert(I);
+    append_range(Worklist, I->operands());
----------------
mkazantsev wrote:
> 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.
You are correct. What limit do you propose, just arbitrary number of processed instructions?


================
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)
----------------
mkazantsev wrote:
> Maybe add statistic to count how many freezes we add?
ok, will do.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:618
+      Result = FI;
+    V->replaceUsesWithIf(FI, [&](Use & U)->bool { return U.getUser() != FI; });
+  }
----------------
mkazantsev wrote:
> Erase/mark trivially dead instructions for erasing?
there is no dead instructions here. V will remains as it has FI as a user.


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

https://reviews.llvm.org/D146699