[PATCH] D147752: [GuardUtils] Add asserts about loop varying widenable conditions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I like the general idea, but using `L->isLoopInvariant` might be too strict, because it can return `false` for things that are in fact invariant (just computed inside the loop). Can we find a better API for it?



================
Comment at: llvm/lib/Transforms/Utils/GuardUtils.cpp:116
+    auto *L = LI.getLoopFor(WidenableBR->getParent());
+    assert(L->isLoopInvariant(WidenableBR->getCondition()) &&
+           "Loop invariant widenable condition made loop variant!");
----------------
This might be too strict. `L->isLoopInvariant` is only checking placement of instruction. Imagine you have invariant `a` and `b` and `WidenableBr->getCondition() = add a, b`. It is still invariant, no matter if the actual `add` instruction is placed inside of loop or not. Or, for example, condition is a load marked as !invariant_load; it's still same value no matter where we move it.

If we had SCEV, the good approximation for what we want is `isAvailableAtLoopEntry`; we need something similar with instructions.

I mean, we *can* go this strict way, but be ready that it will fail when there is no real bug.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147752