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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
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:
> 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?


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

https://reviews.llvm.org/D136233