[PATCH] D136233: [SimpleLoopUnswitch] Inject loop-invariant conditions and unswitch them when it's profitable

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3089-3090
+  DenseMap<Value *, SmallVector<CompareDesc, 4> > CandidatesULT;
+  for (auto *DTN = DT.getNode(Latch); L.contains(DTN->getBlock());
+       DTN = DTN->getIDom()) {
+    ICmpInst::Predicate Pred;
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > apilipenko wrote:
> > > mkazantsev wrote:
> > > > apilipenko wrote:
> > > > > Why do you limit the transform to conditions that dominate the latch?
> > > > Because I prove implications, and the implication can only be proven from something that must execute before the implied condition.
> > > But what you need in fact is dominance between the two branches, not between the branches and the latch.
> > > 
> > > BTW, do you need to worry about implicit control flow/must execute property here?
> > All branches that dominate the latch also dominate each other (if traversed up-down), because they all are in the same path in dom tree (specifically from latch to header).
> > BTW, do you need to worry about implicit control flow/must execute property here?
> 
> No, all that matters is that we work with branches that all dominate the latch (and therfore must execute if backedge is taken). Implicit control flow such as experimental.guard is not supported (and hopefully we can get rid of it).
Can you add a comment explicitly stating that you need one condition to dominate another but you are looking for a stronger property - all conditions dominate the latch. 


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3099-3101
+    // Skip branches that have already been unswithed this way.
+    if (Term->hasMetadata("llvm.invariant.condition.injection.disabled"))
+      continue;
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > apilipenko wrote:
> > > mkazantsev wrote:
> > > > apilipenko wrote:
> > > > > Why do you need to remember this fact and disable unswitching later?
> > > > If the invariant condition is false, the initial loop-variant condition cannot be proven true or false. Example:
> > > > 
> > > > ```
> > > > for (int i = 0; i < N; i++) {
> > > >   x = arr[i];
> > > >   if (x <0 || x >= 128)
> > > >     deopt();
> > > >   if (x < 0 || x >= len)
> > > >     deopt();
> > > >   ...
> > > > }
> > > > ```
> > > > The transform will insert loop-invariant condition `128 <= len` to get rid of loop-variant check `  if (x < 0 || x >= len)` in one of unswitched copies.
> > > > 
> > > > If we have proven that `128 <= len`, then we have proven that `0 <= x < 128 <= len`, and therefore in the unswitched version 2nd check can be removed. But if we haven't proven that, we do not automatically know anything about 2nd condition. Example: len = 100 but x is still less than 99.
> > > > 
> > > > It means that in the unswitched copy, we should preserve the initial loop-variant condition. And we need to prevent from the optimization go crazy and infinitely unswitch on it.
> > > The fact that you are relying on metadata for this is a bit concerning. There might be a pass that doesn't know anything about this metadata and drops it, leading to an undesired unswitching.
> > > 
> > > You can probably check for the invariant condition you are about to insert in the dominating conditions. If there such a condition is known to be false -> don't unswitch. But this won't be foolproof either, as the condition can be rewritten in a way that we can't infer it anymore. 
> > The metadata will reliably protect us from single unswitching run go wild and create infinite number of loops. If the condition was known false between two different unswitchings, why it wasn't optimized away?
> I think we can use SCEV for this, but it's potentially CT-costly. How much are you concerned?
If this is mainly to prevent infinite loop within one unswitch invocation, please add a comment explicitly stating this.


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

https://reviews.llvm.org/D136233