[PATCH] D114155: [LoopAccessAnalysis][SVE] Bail out for scalable vectors

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm marked 3 inline comments as done.
peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1106
+  if (AllocSize.isScalable()) {
+    // Scalable types not yet supported - give up.
+    return 0;
----------------
sdesmalen wrote:
> I think there are three possible scenarios to consider:
> 
> 1. The pointer has a scalable stride (e.g. the PHI is updated with `getetelemenptr <vscale x 2 x i64>, <vscale x 2 x i64>*, i64 1` each iteration of the loop), at which point the Step will not be a SCEVConstant, but rather a SCEVUnknown.
> 2. Case 1, but LAA has replaced the symbolic stride by 1 under the predicate that vscale = 1.
> 3. The case you have in your tests, where the pointer stride is actually fixed (e.g. Step = `{nullptr, +, 1}`) and  the object is scalable.
> 
> For case 1, the dyn_cast on line 1096 would fail. That means the code is already conservative.
> For case 2, we could do some extra work to recognize there is a predicate that checks that vscale == 1, and use `getKnownMinSize()` and allow the code-path here to continue.
> For case 3, we can bail out and no further work is required (meaning that the 'yet' can be removed from the comment).
> 
> Sorry if I'm being a bit nitpicky, but I think the 'not yet supported' is already the case today, and your comment is a bit misleading in that the 'yet' at this point in the code only applies to 2.
> 
> Maybe it's worth capturing these scenarios somewhere in a comment?
Looking at it, I don't think I observed the dyn_cast on 1096 fail. So with respect to your request I'm uncertain exactly what I am documenting right now (all the scalable cases I've tried appear to flow through the new debug statement) and I've chosen to leave that out. Hope that's OK but can revisit if requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114155