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

[Malhar via Phabricator via llvm-commits]

malharJ added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:779
                                 RunningDepId, ASId, ShouldCheckWrap, false)) {
+        UncomputablePtrs.insert(Access.getPointer());
         LLVM_DEBUG(dbgs() << "LAA: Can't find bounds for ptr:"
----------------
fhahn wrote:
> If computing the bounds fails here, we may retry creating checks by adding assumptions below (see line 806). 
> 
> I think it could happen that we have multiple uncomputable pointer bounds here, but for some of them we may be able to actually compute bounds below. Should we remove the ones we can compute bounds below from the set?
Done. thanks for that suggestion.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2179
+      if (auto *I = dyn_cast<Instruction>(Ptr))
+        ORE->emit(recordAnalysis("UnknownArrayBounds", I)
+                  << "Unable to determine aliasing information because the "
----------------
david-arm wrote:
> nit: Maybe it's worth moving the recordAnalysis call to before the `for` loop, i.e.
> 
>   OptimizationRemarkAnalysis R = recordAnalysis("UnknownArrayBounds", I);
>   for (...)
> 
> 
Unfortunately that can't be done because `Instruction* I` is being declared inside the for-loop.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/memory-dep-remarks.ll:12
+
+; CHECK: remark: source.c:4:16: cannot identify array bounds
+
----------------
fhahn wrote:
> malharJ wrote:
> > sdesmalen wrote:
> > > 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.
> > If you see the earlier version/diffs of the patch, the reporting was being done in LoopVectorizer.
> > That time I had added "loop not vectorized" in the tests.
> > 
> > But now that it has moved to LoopAccessAnalysis, we are no longer emitting "loop not vectorized" here.
> > That prefix is prepended by the function reportVectorizationFailure() in LoopVectorize.cpp
> > 
> > For the latter part, I have made the change.
> >> Unable to determine aliasing information because the bounds of this access cannot be computed.
> 
> I'm not sure this is entirely accurate. AFAIK  uncomputable bounds here mean we cannot generate runtime checks.
> 
> 'aliasing information' seems ambiguous here, because at the point where we check whether the bounds are computable, we already determined that the access may alias another access, independent of whether the bounds can be computed or not (like the underlying objects may alias).
Ok I agree.

I have changed the remark accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371