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

Ayal Zaks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 19 04:31:18 PST 2018


Ayal added inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:223
     return ShouldRetryWithRuntimeCheck &&
-           Status == VectorizationSafetyStatus::Unsafe;
+           Status == VectorizationSafetyStatus::SafeWithRtChecks;
   }
----------------
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...).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55798





More information about the llvm-commits mailing list