[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:
> > > > 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.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D55798