[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:102
+  enum class StatusTy {
+    // Can vectorize safely without RT checks.
+    Safe,
----------------
// Can vectorize safely without RT checks. All dependences are known to be safe.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:104
+    Safe,
+    // Can vectorize with RT checks.
+    SafeWithRtChecks,
----------------
// Can vectorize with RT checks to overcome unknown dependencies.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:106
+    SafeWithRtChecks,
+    // Cannot vectorize, because we found unknown or known unsafe dependences.
+    Unsafe,
----------------
// Cannot vectorize due to known unsafe dependencies.
, "unknown" is considered SafeWithRtChecks, right?


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:206
   /// vectorization.
-  bool isSafeForVectorization() const { return SafeForVectorization; }
+  bool isSafeForVectorization() const { return Status == StatusTy::Safe; }
 
----------------
Use this `isSafeForVectorization()` getter method where the `SafeForVectorization` flag was read before, below, instead of replacing it with (inlined) comparisons of `Status == StatusTy::Safe`.

This will also make it useful (though it could be made protected/private).


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:287
+  /// be unsafe.
+  StatusTy Status;
 
----------------
Perhaps use a more specific name; e.g., `VectorizationSafetyStatus`?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1233
   case Unknown:
+    return StatusTy::SafeWithRtChecks;
   case ForwardButPreventsForwarding:
----------------
Consider splitting into an NFC patch which has only Safe and Unsafe states, with the test showing it currently vectorizes; plus a separate patch adding SafeWithRtChecks state with updated test.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1323
+void MemoryDepChecker::mergeInStatus(StatusTy S) {
+  switch (Status) {
+  case StatusTy::Safe:
----------------
Could this be done as follows, based on the numerical values of the enum, properly sorted as now?

```
  if (Status < S)
    Status = S;

```



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

https://reviews.llvm.org/D54892