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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/metadata-width.ll:55
+!3 = !{!"llvm.loop.vectorize.width", !4}
+!4 = !{i32 8, i32 0}
----------------
sdesmalen wrote:
> david-arm wrote:
> > 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 ?
> The proof of concept for scalable loop vectorization (D90343) depends on this patch and shows the `isScalable = true` case does something sensible with this information. Hopefully that is sufficient for now while we work on actually implementing scalable auto-vec support in the LoopVectorizer.
Hm, I am a bit reluctant to add something to LangRef which causes passes to crash on valid IR. Would it be possible to either convert the scalable VF into a fixed one (replacing n with 1 should be valid?), or just bail out on a scalable VF, without too much effort?


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

https://reviews.llvm.org/D88962