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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:586
 
+  std::shared_ptr<Value *> getUncomputablePtr() { return UncomputablePtr; }
+
----------------
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?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1779
               if (Type != Dependence::NoDep)
                 Dependences.push_back(Dependence(A.second, B.second, Type));
 
----------------
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.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1861
+  DebugLoc Loc;
+  if (I) {
+    Loc = I->getDebugLoc();
----------------
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).


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1864-1866
+    if (auto *DD = dyn_cast<Instruction>(isa<MemSetInst>(I)
+                                             ? cast<MemSetInst>(I)->getRawDest()
+                                             : getPointerOperand(I)))
----------------
This seems like a case that should be added to `getPointerOperand` ?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2110
+    if (auto Ptr = Accesses.getUncomputablePtr()) {
+      if (auto *I = dyn_cast<Instruction>(*Ptr)) {
+        recordAnalysis("UnknownArrayBounds", I)
----------------
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?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2111
+      if (auto *I = dyn_cast<Instruction>(*Ptr)) {
+        recordAnalysis("UnknownArrayBounds", I)
+            << "cannot identify array bounds";
----------------
Just leave the name as `CantIdentifyArrayBounds` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371