[PATCH] D54892: [LAA] Avoid generating RT checks for known deps preventing vectorization.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:212
+  bool shouldRetryWithRuntimeCheck() {
+    return RuntimeChecksFeasible && ShouldRetryWithRuntimeCheck;
+  }
----------------
anemet wrote:
> fhahn wrote:
> > anemet wrote:
> > > What is the actual semantical difference between RuntimeChecksFeasible and ShouldRetryWithRuntimeCheck?
> > > 
> > > In other words, can't we just clear ShouldRetryWithRuntimeCheck when we see an unsafe known dep?
> > At the moment, they refer to slightly different things: RuntimeChecksFeasible is true iff we did not find any dependences making RT ineffective, ShouldRetryWithRuntimeChecks is true iff we encountered an unknown dependence where RT checks might help. Additionally we only set ShouldRetryWithRuntimeChecks for some Unknown dependences. The reason I went with an additional flag was that it seemed easier to see what is going on.
> > 
> > Currently clearing ShouldRetryWithRuntimeCheck would not be enough I think, because we keep checking dependences and we could set ShouldRetryWithRuntimeChecks again for later dependences. Changing the code to bail out early would help (see discussion below)
> > 
> > 
> Even then I would prefer a single three-state value rather than a two booleans, e.g. something like: true, false and unfeasible/failed or some such.
Agreed it sounds like RetryWithRuntimeChecks could be enum'd to indicate


  # Should: unknown dependencies exist where RT checks might help;
  # Don't - Won'tHelpTo: dependencies exists which even RT checks cannot help; or
  # Don't - NoNeedTo: all's well w/o RT checks.

OTOH, if we encounter a dependence that prevents vectorization and can't bail out on the spot (see below), we could raise an UnsafeForVectorization flag instead. Or perhaps SafeForVectorization/areDepsSafe() should be/return the single three-state value, e.g., something like: Safe, Unsafe, SafeWithRT which correspond to (3), (2), (1) respectively? Start optimistically Safe, move to SafeWithRT only from Safe mode, keep Unsafe is sticky.

(BTW, isSafeForVectorization() seems to be dead and should be removed. The return value of areDepsSafe() is used instead.)


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1661
+            // the runtime checks at compile time.
+            RuntimeChecksFeasible &= Type == Dependence::Unknown || DepSafe;
 
----------------
fhahn wrote:
> Ayal wrote:
> > Suggest to add and call `Dependence::isUnsafeForVectorization(Type)` which will determine if there's a dependence that cannot be overcome by runtime checks. Perhaps find a better name - it implies but is not equivalent to `!isSafeForVectorization(Type)`.
> > 
> > Then bail-out as soon as such a dependence is encountered; there's no point in continuing to `RecordDependences`.
> > 
> > This could alternatively be done here as follows, by leveraging `ShouldRetryWithRuntimeCheck`, which essentially already indicates if runtime checks may be useful (but then early bail-out is necessary):
> > 
> > 
> > ```
> >             if (!DepSafe && Type != Dependence::Unknown) {
> >               ShouldRetryWithRuntimeCheck = false;
> >               RecordDependences = false;
> >               Dependences.clear();
> >               LLVM_DEBUG(dbgs() << "Found unsafe dependence\n");
> >               return false;
> >             }
> > ```
> > 
> 
> > Suggest to add and call Dependence::isUnsafeForVectorization(Type) which will determine if there's a dependence that cannot be overcome by runtime checks. Perhaps find a better name - it implies but is not equivalent to !isSafeForVectorization(Type).
> 
> Thanks, I'll add a special function. One thing I just realized is that we only should do RT checks for some Unknown dependences. It might be worth adding a new type UnknownRTCheckable or something, to even better distinguish when RT checks are profitable.
> 
> > Then bail-out as soon as such a dependence is encountered; there's no point in continuing to RecordDependences.
> 
> The reason I did not go for an early bailout is that we keep recording dependences even if we found some unsafe ones, I suppose for better reporting. If we go for an early bailout in this patch, this seems slightly inconsistent and it might be worth changing to an early bailout in case of unsafe deps too first?
> Then bail-out as soon as such a dependence is encountered; there's no point in continuing to RecordDependences.

Ahh, that's true for LV, but not for LoopDistributor which would like to obtain all the dependencies in order to separate the unsafe ones...


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

https://reviews.llvm.org/D54892