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

Sander de Smalen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 10 04:12:24 PST 2020


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:
> > 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).
> > 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.
> 
> That makes sense, my suggestion was to just convert the scalable user VF to a fixed one after getting the hint instead of crashing. As in
> 
> ```
>    // Get user vectorization factor and interleave count.
>    ElementCount UserVF = Hints.getWidth();
> +  if (UserVF.isScalable()) {
> +    // TODO: Use scalable user VF, once LV is ready. For now, just assume n == 1.
> +    UserVF = ElementCount::getFixed(UserVF.getKnownMinValue());
> +  }
>    unsigned UserIC = Hints.getInterleave();
> 
> ```
> 
> Only `processLoopInVPlanNativePath` and `processLoop` would need to be updated I think. as we are free to drop the hints, I think that would be preferable to crashing and should be legal?
> 
Okay, so you mean assuming Fixed for now and then re-enable this in e.g. D91077 (which adds scalable vector support for some simple loop, but which doesn't yet guarantee that it won't crash on all inputs).
Yes, I think that makes sense.


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

https://reviews.llvm.org/D88962



More information about the llvm-commits mailing list