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

[Malhar via Phabricator via llvm-commits]

malharJ added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:633
 
+  std::unique_ptr<OptimizationRemarkEmitter> ORE;
+
----------------
sdesmalen wrote:
> This seems unused?
thanks for pointing it out. Removed it.


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

But we are only emitting (as a remark) the first one found. 


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2056
+
+    if (auto Ptr = Accesses.getUncomputablePtr()) {
+      if (auto *I = dyn_cast<Instruction>(*Ptr)) {
----------------
sdesmalen wrote:
> `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.
Ok, I agree but there's an issue here .. 

LoopAccessInfo::recordAnalysis() cant be called from within the function ( AccessAnalysis::canCheckPtrAtRT() ).
as it's not a member of AccessAnalysis class. 

Perhaps one way to do it would be for AccessAnalysis::canCheckPtrAtRT() to accept a parameter
by reference and then we can use the value after the call as input to recordAnalysis() here.
But I'm afraid this approach is not much less verbose than current approach pf having UncomputablePtr as a member variable. 


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2123
+
+    OptimizationRemarkAnalysis &R =
+        recordAnalysis("UnsafeDep", Dep.getDestination(*this))
----------------
sdesmalen wrote:
> 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.
Thanks for pointing this out. Done.


================
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:
> sdesmalen wrote:
> > nit: Please remove the newline.
> Instead of using `+` to concatenate two strings, let this be handled by raw_ostream using `<<` .
can we keep this newline ?

The current text (Note: it was not introduced by this patch) is too long already:

> 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




================
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:
----------------
sdesmalen wrote:
> Please CHECK-NEXT for the full line that is printed.
Agreed.

But now the code will only print the remaining text: "Memory location is the same as ...etc"
when debug info is available.

so this is no longer an issue (since this test does not contain any debug info/metadata).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371