[PATCH] D88962: [SVE] Add support for scalable vectors in vectorize_width loop attribute

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

What happens if w



================
Comment at: llvm/docs/LangRef.rst:5933
 
-This metadata sets the target width of the vectorizer. The first
-operand is the string ``llvm.loop.vectorize.width`` and the second
-operand is an integer specifying the width. For example:
+The vector width is an ElementCount tuple, where the first value specifies the
+minimum vector width and the second value indicates whether the vectorization
----------------
ElementCount is an internal name of the LLVM codebase I guess and nothing defines it in LangRef? I think it  would be better to drop it, because here it is just a regular metadata tuple and you already outline what the fields hold.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:63
+    Hint(const char *Name, ElementCount EC)
+        : Name(Name), Value{0}, Kind(HK_WIDTH) {
+      Value.EC = EC;
----------------
nit: using `{0}` seems inconsistent with the other braces used here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:66
+  else if (const MDNode *MD = dyn_cast<MDNode>(Arg)) {
+    if (Kind != HK_WIDTH || MD->getNumOperands() != 2)
+      return false;
----------------
might be good to a comment that we are looking for a (width, isScalable) tuple here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:261
   const ConstantInt *C = mdconst::dyn_extract<ConstantInt>(Arg);
-  if (!C)
+  if (!C && Name != Width.Name)
     return;
----------------
`C` is only used in the condition, maybe just check `mdconst::dyn_extract<ConstantInt>(Arg)` in the if?

But now that we pass the metadata node to validateAndSet, do we actually need the check here or can we just do all the validation in the `validateAndSet`? Seems like the time-savings by existing a bit earlier would be very minor.


================
Comment at: llvm/test/Transforms/LoopVectorize/metadata-width.ll:55
+!3 = !{!"llvm.loop.vectorize.width", !4}
+!4 = !{i32 8, i32 0}
----------------
david-arm wrote:
> david-arm wrote:
> > sdesmalen wrote:
> > > nit: `s/i32 0/i1 false/`
> > If I make this change the test fails. We will still always print out "i32 0". Is there a way to control the format generated?
> Sorry I just realised this is an input to the test, rather than testing the output! Please ignore my comment.
also needs a test which actually sets `isScalable` to `true`?


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

https://reviews.llvm.org/D88962