[PATCH] D55798: [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 inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:223
     return ShouldRetryWithRuntimeCheck &&
-           Status == VectorizationSafetyStatus::Unsafe;
+           Status == VectorizationSafetyStatus::SafeWithRtChecks;
   }
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > Ayal wrote:
> > > > > > When SafeWithRtChecks is added, in next patch, consider renaming the flag or method (slightly), as they will no longer mean exactly the same thing. 
> > > > > 
> > > > > Consider renaming `ShouldRetryWithRuntimeCheck` to something like `FoundDependenceForRuntimeCheck`; or keep the original name but fold SafeWithRtChecks into the flag, i.e., when turning the flag on check if Status is not Unsafe, and when setting Status to Unsafe turn the flag off.
> > > > > 
> > > > > On second thought, now that Status includes SafeWithRtChecks, this flag seems redundant, and suffice to have
> > > > > 
> > > > > ```
> > > > > bool shouldRetryWithRuntimeCheck() const {
> > > > >   return Status == VectorizationSafetyStatus::SafeWithRtChecks;
> > > > > }
> > > > > 
> > > > > ```
> > > > > ?
> > > > Currently, ShouldRetryWithRuntimeCheck is only set when we return Unknown for non-constant distances between dependences. There are other cases where we return Unknown, but do not set it.
> > > > 
> > > > By removing it, we would generate runtime checks in more cases. That might be beneficial, but I think should be done in a separate patch. 
> > > Agreed, there's no intent to generate runtime checks in more cases, and the ShouldRetryWithRuntimeCheck flag is only set when a dependence having non constant distance is encountered, w/o being unset when any other (unknown) dependence is encountered. In other words, ShouldRetryWithRuntimeCheck means FoundNonConstantDistanceDependence.
> > > 
> > > Ideally, `SafeWithRtChecks` would indicate that only (Safe or) Unknown dependences which can be overcome by RT checks exist, as documented above. This however is not easily determined - "ShouldRetry" may fail. Furthermore, `isSafeForVectorization(DepType Type)` has only the Type to consider, which does not even distinguish between non constant distance dependences and other RT-un/friendly Unknowns; hence the method returns `SafeWithRtChecks` for all Unknowns. In other words, `SafeWithRtChecks` means `PossiblySafeWithRtChecks`.
> > > 
> > > So conceptually Status has four possible states of interest:
> > > 
> > >   # Safe --> No need to Retry
> > >   # PossiblySafeWithRtChecks without having found a non constant distance --> Don't Retry, not worth it(?)
> > >   # PossiblySafeWithRtChecks having found a non constant distance --> Retry
> > >   # Unsafe --> Don't Retry, no point
> > > 
> > > In order to determine the current Status, `isDependent()` communicates to `isSafeForVectorization(DepType Type)` a DepType. One possible option is to introduce another 'NonConstantDistance' member to DepType, but that might be intrusive. Another option is to have `isDependent()` return both a DepType and an indication whether a non constant distance dependence was observed, and determine the Status according to both.
> > > 
> > > In any case, `areDepsSafe()` can return a Status instead of a boolean; this could simplify the `if (!CanVecMem && DepChecker->shouldRetryWithRuntimeCheck())` (although this !CanVecMem seems redundant...).
> > Agreed. What do you think of proceeding in the following steps:
> > 
> >   1. change SafeWithRtChecks to PossiblySafeWithRtChecks in this patch
> >   2. in a follow up patch, add NonConstantDistance to DepType, drop the possibly from PossiblySafeWithRtChecks and return it for NonConstantDistance dependences and Unsafe otherwise
> >   3. in another follow up patch, make areDepsSafe return a Status
> > 
> > IMO splitting it up makes sense, as the follow ups are structural improvements not directly related to the aim of this patch and it makes it easier to pin point where things went wrong, if there are problems.
> > 
> Sure, this breakdown LGTM. Some comments:
> 
> 
>   # When introducing PossiblySafeWithRtChecks, suggest to rename ShouldRetryWithRuntimeCheck to FoundNonConstantDistanceDependence. Or keep the name but turn the flag off whenever Status becomes Unsafe. The original motivation is to avoid having discrepancy between a flag and a method of exactly the same name, where the method no longer simply retrieves the flag.
> 
>   # When to introducing NonConstantDistance, if we return Unsafe for all other Unknown's we'll be restricting Retry only to cases where all dependences are (either Safe or) of non constant distance. Seems Retry may succeed for other Unknown dependences today, say of distinct types/strides (but not of distinct address spaces - consider these Unsafe?). Would be good btw to include tests for various Unknown combinations, and perhaps a TODO for cases that have only (Safe and) non-constant distance unknown dependences.
> 
>   # Changing the return type of areDepsSafe is indeed orthogonal.
> When to introducing NonConstantDistance, if we return Unsafe for all other Unknown's we'll be restricting Retry only to cases where all dependences are (either Safe or) of non constant distance. Seems Retry may succeed for other Unknown dependences today, say of distinct types/strides (but not of distinct address spaces - consider these Unsafe?). Would be good btw to include tests for various Unknown combinations, and perhaps a TODO for cases that have only (Safe and) non-constant distance unknown dependences.

Yep, before making that change, we have to make sure we do not miss any cases where runtime checks could be succeed. I'll put up a patch once I did that.


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

https://reviews.llvm.org/D55798