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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

PTAL.



================
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:
> skatkov wrote:
> > 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.
> > 
> Maybe add a TODO comment like "move to utils"?
Let's keep it as is for a while.


================
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:
> skatkov wrote:
> > 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.
> For invoke, you can still freeze its result in normal path. But I see the point.
Added a test and fixed a bug, thanks.

The test is added to posion.ll where for invoke we cannot insert it to normal path due to phi node user..


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:606
+    }
+    DropPoisonFlags.insert(I);
+    append_range(Worklist, I->operands());
----------------
mkazantsev wrote:
> skatkov wrote:
> > 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?
> I dunno... Do you have data on downstream CT impact? Might not be a big deal, after all.
For a while, I do not see any problems. If they appear, we'll add some 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)
----------------
skatkov wrote:
> mkazantsev wrote:
> > Maybe add statistic to count how many freezes we add?
> ok, will do.
added


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

https://reviews.llvm.org/D146699