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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/metadata-width.ll:55
+!3 = !{!"llvm.loop.vectorize.width", !4}
+!4 = !{i32 8, i32 0}
----------------
fhahn wrote:
> 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?
> 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?
That would lead to a bit of a chicken/egg problem, because we're adding this feature so we can incrementally make the loop vectorizer cope with scalable vectors. There are currently too many code-paths in there to fix up in one go. D90343 uses this metadata to enable vectorization of an individual loop with scalable vectors and gives us a route to enable and test individual loops in unit tests or in existing code (using the Clang attribute in D89031). The fact that the vectorizer currently crashes on most inputs, is a bug, it just happens to be a bug we know about. It is also a temporary broken state that we will want fix as soon as possible.

Is it not possible to add a line to the LangRef saying that this is experimental until the loop vectorizer supports scalable vectors? (and we remove that line when the vectorizer is stable enough).


================
Comment at: llvm/test/Transforms/LoopVectorize/no_array_bounds2.ll:3
+
+
+;  #pragma clang loop vectorize(enable)
----------------
Can you add a description of what this file is testing?

For the file name itself, is there a better name than `no_array_bounds2.ll` ?


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

https://reviews.llvm.org/D88962