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

[David Sherwood via Phabricator via llvm-commits]

david-arm marked an inline comment as done.
david-arm added inline comments.


================
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;
----------------
fhahn wrote:
> nit: using `{0}` seems inconsistent with the other braces used here?
Good spot. Since I'm initialising Value explictly below I can simply remove "Value{0}, " entirely.


================
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;
----------------
fhahn wrote:
> `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.
Sure, sounds sensible!


================
Comment at: llvm/test/Transforms/LoopVectorize/metadata-width.ll:55
+!3 = !{!"llvm.loop.vectorize.width", !4}
+!4 = !{i32 8, i32 0}
----------------
fhahn wrote:
> 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`?
Unfortunately I can't do that at the moment because the vectorizer crashes when trying to perform scalable vectorisation. This is why I only tested the 'false' case. In theory I could create a negative test that tests the vectoriser crashes, which we could remove later once we support vectorisation. Any thoughts @fhahn or @sdesmalen ?


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

https://reviews.llvm.org/D88962