[PATCH] D108371: [LAA] Add Memory dependence and unknown bounds remarks.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen requested changes to this revision.
sdesmalen added a comment.
This revision now requires changes to proceed.

Hi @malharJ, thanks for cleaning up the patch and making changes based on what we discussed offline, it's a bit more manageable to review now.
There are still some changes that I think are unnecessary, and I found that the patch needs to be rebased (this is how I found some confusing output `test/Analysis/LoopAccessAnalysis/pointer-phis.ll`).



================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:633
 
+  std::unique_ptr<OptimizationRemarkEmitter> ORE;
+
----------------
This seems unused?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1725
+                                         VectorizationSafetyStatus::Safe)
+              UnsafeDependence = std::make_shared<Dependence>(
+                  Dependence(A.second, B.second, Type));
----------------
Can there only be a single unsafe/unknown dependence? Or can there be more?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2056
+
+    if (auto Ptr = Accesses.getUncomputablePtr()) {
+      if (auto *I = dyn_cast<Instruction>(*Ptr)) {
----------------
`UncomputablePtr` and the mechanism around it seems entirely redundant, because it doesn't add any information that is used anywhere. The only reason that UncomputablePtr is collected is to avoid printing `"Cannot identify array bounds"` when it gets to the then-block of  `if (!CanDoRTIfNeeded) { ... }`. The information is redundant, because when UncomputablePtr is set, then CanDoRT is false, and vice-versa.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2059
+        recordAnalysis("UnknownArrayBounds", I)
+            << "Cannot identify array bounds";
+      }
----------------
nit: The capitalisation of `cannot -> Cannot` seems unnecessary.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2093
         CanVecMem = false;
+        recordAnalysis("CantCheckMemDepsAtRunTime")
+            << "cannot check memory dependencies at runtime";
----------------
unnecessary change.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2123
+
+    OptimizationRemarkAnalysis &R =
+        recordAnalysis("UnsafeDep", Dep.getDestination(*this))
----------------
These changes here obfuscate the report that's generated by LAA when running `loop(print-access-info)`. The information printed was:

``` Loop access info in function 'store_with_pointer_phi_incoming_phi':
   loop.header:
  The compiler can't determine the cause of the issue.
     Dependences:
       Unknown:
           %v8 = load double, double* %arrayidx, align 8 ->
           store double %mul16, double* %ptr.2, align 8
```

The information that is added by the remark is:
```Loop access info in function 'store_with_pointer_phi_incoming_phi':
  loop.header:
    Report: unsafe dependent memory operations in loop. Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
Unknown data dependence. Memory location is the same as accessed at <UNKNOWN LOCATION>. 
 The compiler can't determine the cause of the issue.
    Dependences:
      Unknown:
          %v8 = load double, double* %arrayidx, align 8 ->  
          store double %mul16, double* %ptr.2, align 8
```

The extra information here isn't particularly useful, especially because it doesn't have any line information. Perhaps the code should disable the extra info if the location is unknown/unavailable.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2136
+    case MemoryDepChecker::Dependence::Backward:
+      R << "\nBackward loop carried data dependence. " + MemlocText
+        << ore::NV("Location", SourceLoc);
----------------
nit: Please remove the newline.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2136
+    case MemoryDepChecker::Dependence::Backward:
+      R << "\nBackward loop carried data dependence. " + MemlocText
+        << ore::NV("Location", SourceLoc);
----------------
sdesmalen wrote:
> nit: Please remove the newline.
Instead of using `+` to concatenate two strings, let this be handled by raw_ostream using `<<` .


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/stride-access-dependence.ll:122
 ; CHECK-NEXT:     Report: unsafe dependent memory operations in loop
+; CHECK-NEXT:     Backward loop carried data dependence.
 ; CHECK-NEXT:     Dependences:
----------------
Please CHECK-NEXT for the full line that is printed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371