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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

The fix looks valid, but maybe you can update the comment a bit to better describe the different scenarios.



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1106
+  if (AllocSize.isScalable()) {
+    // Scalable types not yet supported - give up.
+    return 0;
----------------
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?


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/scalable-vector-regression-tests.ll:23
+  br i1 false, label %2, label %1
+1:
+  ret i1 false
----------------
nit: can you give these names instead of numbers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114155