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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
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!");
----------------
mkazantsev wrote:
> 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.
> 
This is true, but it would be a pass-ordering issue. LICM should have hoisted such IR out. 
We can strengthen against pass-ordering, but to what extent? In short, failing the assert shows we have one of 2 problems: either an actual bug or a missed performance opportunity, both of which needs investigation :) 
I think we can use `canHoistOrSinkInstruction` API from LICM if we want to have lesser false positives, but that API needs AA to make sure we are hoisting correctly in presence of AA. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147752