[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/include/llvm/Analysis/LoopAccessAnalysis.h:261
 
+  const SmallVector<Dependence, 2> &getUnsafeDependences() const {
+    return UnsafeDependences;
----------------
malharJ wrote:
> fhahn wrote:
> > There's no need to return a SmallVector with the length encoded, the callers should not care about that. You can use ArrayRef if the callers do not add to the vector or SmalLVectorImpl if they need to add elements.
> Currently the code uses "auto" when accessing the. value returned by this getter
> so there isn't a need for the user to know the size used in the template ...
> 
> regardless, I've changed it to use SmallVector<T> instead if that's ok.
If there is no need to modify the array returned by getUnsafeDependences (which there isn't, because the returned value is `const`), ArrayRef seems the better class to use, because it has all sorts of convenient utilities and is similarly just a (immutable) reference to the array.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2074
         CanVecMem = false;
+        FailReason = FailureReason::UnsafeDataDependenceTriedRT;
         return;
----------------
Is this case not already covered by the `recordAnalysis` call above?

It seems like a mechanism for doing this already exists (recordAnalysis), it may be worth extending that to handle multiple reports (there is currently a limitation that it uses a single `Report` variable, but perhaps that could be made into a vector of reports which can be appended to).



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2120
+    const auto &UnsafeDependences = getDepChecker().getUnsafeDependences();
+    unsigned NumUnsafeDeps = UnsafeDependences.size();
+    assert(NumUnsafeDeps > 0 && "expected unsafe dependencies but found none");
----------------
nit: single-use variable, can be inlined in the next statement.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2177-2178
+  case FailureReason::UnknownArrayBounds: {
+    // add detailed remarks at locations of pointers where bound cannot
+    // be computed
+    for (Value *Ptr : getUncomputablePtrs())
----------------
nit: please start your comments with capitalisation and end with a period.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/memory-dep-remarks.ll:12
+
+; CHECK: remark: source.c:4:16: cannot identify array bounds
+
----------------
Would a prefix like "Loop cannot be vectorized: " be useful?

I think the message is a bit cryptic, how about:

  Unable to determine aliasing information because the bounds of this access cannot be computed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371