[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);
----------------
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"?


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:601
+    if (any_of(I->operands(), [InsertPt](Value *Op) {
+          return isa<Instruction>(Op) && !getFreezeInsertPt(Op, InsertPt);
+        })) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:606
+    }
+    DropPoisonFlags.insert(I);
+    append_range(Worklist, I->operands());
----------------
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.


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

https://reviews.llvm.org/D146699