[PATCH] D108371: [LAA] Add Memory dependence remarks.

[Malhar via Phabricator via llvm-commits]

malharJ marked an inline comment as done.
malharJ added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:304
+  /// Used for generating optimization remarks.
+  std::shared_ptr<Dependence> UnsafeDependence;
+
----------------
fhahn wrote:
> Does this need to be a `shared_ptr`? If you want to encode the fact that it may not be set, using `Optional` may be a better choice. Or you could initialize it to `NoDep` in case there is no unsafe dependence.
I've removed it now based on review comment by sdesmalen, so this is no longer an issue.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:586
 
+  std::shared_ptr<Value *> getUncomputablePtr() { return UncomputablePtr; }
+
----------------
sdesmalen wrote:
> UncomputablePtr doesn't really need a separate variable in AccessAnalysis, since it's only set by one function, and used directly by the function that calls it.
> 
> You can change `canCheckPtrAtRT` to take `Value **UncomputablePtr = nullptr`, and in `canCheckPtrAtRT` assign the value if the value is requested:
> ```if (UncomputablePtr)
>   *UncomputablePtr = Access.getPointer();```
> 
> Where you call it, you can have:
> ```
> Value *UncomputablePtr = nullptr;
> bool CanDoRTIfNeeded =
>     Accesses.canCheckPtrAtRT(*PtrRtChecking, PSE->getSE(), TheLoop,
>                              SymbolicStrides, false, &UncomputablePtr);
> 
> if (!CanDoRTIfNeeded) {
>   if (auto *I = dyn_cast_or_null<Instruction>(UncomputablePtr))
>     recordAnalysis("UnknownArrayBounds", I)
> }
> ```
> 
> Can you split this change out into a separate patch with a test that demonstrates the change?
Done, new patch: https://reviews.llvm.org/D115873


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1779
               if (Type != Dependence::NoDep)
                 Dependences.push_back(Dependence(A.second, B.second, Type));
 
----------------
sdesmalen wrote:
> It shows here that the dependences are already collected. You can iterate `Dependences` to find the dependence that isn't safe, so that you don't have to add `UnsafeDependence` and maintain that separately.
Done. 
That's a good point, thanks !


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1861
+  DebugLoc Loc;
+  if (I) {
+    Loc = I->getDebugLoc();
----------------
sdesmalen wrote:
> I guess this can be more briefly written as:
> 
>   Loc = I->getDebugLoc();
>   if (auto *PtrI = dyn_cast_or_null<Instruction>(getPointerOperand(I)))
>     Loc = PtrI->getDebugLoc();
> 
> (at which point it probably doesn't require a separate function anymore, especially since it has only one use).
I've removed this function and inlined the code since there is only one usage.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2110
+    if (auto Ptr = Accesses.getUncomputablePtr()) {
+      if (auto *I = dyn_cast<Instruction>(*Ptr)) {
+        recordAnalysis("UnknownArrayBounds", I)
----------------
sdesmalen wrote:
> It now passes in `I` as the instruction, but the debug location of the remark seems unchanged in the test. What value is this adding?
This was essentially an issue in the original test,
The debug info (!35) used by the loop was incorrect.
I've changed it to use !32 now.

I have corrected the issue in my new patch:
https://reviews.llvm.org/D115873


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371