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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

Thanks Adam and Ayal, responses inline. I would be happy to update the patch to do an early bailout, but I am not sure if that is desirable, as it might have a negative impact on the reports generated.



================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:212
+  bool shouldRetryWithRuntimeCheck() {
+    return RuntimeChecksFeasible && ShouldRetryWithRuntimeCheck;
+  }
----------------
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)




================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1661
+            // the runtime checks at compile time.
+            RuntimeChecksFeasible &= Type == Dependence::Unknown || DepSafe;
 
----------------
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?


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

https://reviews.llvm.org/D54892