Page MenuHomePhabricator

[LV] Add -scalable-vectorization=<option> flag.
ClosedPublic

Authored by sdesmalen on May 5 2021, 2:01 PM.

Details

Summary

This patch adds a new option to the LoopVectorizer to control how
scalable vectors can be used.

Initially, this suggests three levels to control scalable
vectorization, although other more aggressive options can be added in
the future.

The possible options are:

  • Disabled: Disables vectorization with scalable vectors.
  • Enabled: Vectorize loops using scalable vectors or fixed-width vectors, whichever is most profitable. Favours fixed-width vectors when the cost is a tie.
  • Preferred: Like 'Enabled', but favoring scalable vectors when the cost-model is inconclusive.

Diff Detail

Event Timeline

sdesmalen created this revision.May 5 2021, 2:01 PM
sdesmalen requested review of this revision.May 5 2021, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2021, 2:01 PM

Hey @sdesmalen
Thank you for the patch. Nice to see the famous flag in Phabricator.
I have some comments.
I hope they are not too non-sense.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
132

I don't understand where is this function is used, because you've replaced it by isScalableVectorizationPreferred, but you did not changed in any other place. Maybe I am missing something.
And none of them are used.

151

What will happened when Scalable.Value is SK_Enabled?
It will not be scalable?

155

nit: isScalableVectorizationDisable()

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
108

Do we needs this?
Does this line do the same:
Scalable("vectorize.scalable.enable", ScalableVectorization, HK_SCALABLE),?

Matt added a subscriber: Matt.May 6 2021, 7:11 AM

Hi @CarolineConcatto thanks for having a look.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
132

I'm not entirely sure what you mean.
isScalableVectorizationPreferred is used in disableScalableVectorization, which is used in getWidth and in LoopVectorize.cpp.

151

SK_Enabled means that scalable vectorization can be used, but isn't necessarily preferred over fixed-width. In contrast, the purpose of this method is to answer if scalable vectorization is actually preferred.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
108

This is necessary when both the flag and the attribute are set (through code pragmas). After this change, if the user-code contains pragmas such as vectorize_width(4, scalable), then having -scalable-vectorization=off will cause the LV to override the hint so that scalable vectors are not considered.

sdesmalen updated this revision to Diff 343640.May 7 2021, 4:04 AM
  • Added SK_Undefined to fix the case where 'vectorize.width' is specified, but 'vectorize.scalable.enable' is not. In the previous revision of this patch, it would default to the value of the flag, which when set to 'preferred' would cause loops with #pragma clang loop vectorize_width(4) to be vectorized with VF=vscale x 4. This is now resolved (and tested).
  • Also renamed disableScalableVectorization -> isScalableVectorizationDisabled.

Hey @sdesmalen
I have more questions about the implementation.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
150

This function is not used.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
86

Should it be all the possible solutions here:
Val == SK_Disabled
Val == SK_Preferred
Val == SK_Enabled
Val == SK_Undefined

97–98

Can vectorize.scalable.enable have the same values as scalable-vectorization?
Which values should LoopVectorizeHints::Hint::validate check?

115

Just to double check:
if Scalable.Value != SK_Undefined and ScalableVectorization != SK_Disabled
Scalable.value will have the value set in vectorize.scalable.enable, is that correct?
and not in
scalable-vectorization

llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-analysis.ll
14

Is there a way to tell that VF scalable will not be used?
Only this:
LV: Found feasible scalable VF
is quite odd, because it does not explicitly says it will not use scalable.

sdesmalen marked 4 inline comments as done.May 10 2021, 9:06 AM
sdesmalen added inline comments.
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
150

Okay, I can move this to the other patch.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
86

Perhaps at some point, but at the moment the value is interpeted as a boolean, and Clang sets it only to '0' or '1', so if we want to expand it to support other values in the metadata, we'll need to update Clang as well (which is something or another patch).

97–98

So first the variable Scalable is initialized with SK_Undefined.

  • Then it looks through the metadata to see if the front-end has set the value (which can only be 'disabled' or 'preferred', which is checked by LoopVectorizeHints::Hint::validate)
  • If it is not set by the metadata, then it will see if the flag is set to force scalable vectorization one way or another.
  • Otherwise, if it is set by the metadata, it can be overriden by the flag only if the value is 'disable'.

This means it is possible to disable scalable vectorization using a single flag. But otherwise, metadata (because it is more specific) takes precedence over the flag (which is more generic).

115

Yes, that's correct. In that case it will have the value from the metadata.

llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-analysis.ll
14

It has the -NOT as part of the check prefix though.

e.g. CHECK-NOT: foo
will cause FileCheck to fail if foo occurs.

In this case, the prefix is CHECK_SCALABLE_DISABLED instead of CHECK, so CHECK_SCALABLE_DISABLED-NOT fails if LV: Found feasible scalable VF occurs.

vkmr added a comment.May 11 2021, 5:52 AM

Thanks @sdesmalen for the patch!
Just a couple of clarifying questions.
Also, all the non-target specific test cases only check for -scalable-vectorization=on and with metadata hint enabled (except for one that focuses on width hint). Just for the sake of completeness, more tests with different values for the flag and its combination with the metadata hint would be nice to have.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
108

The ordering of SK_Enabled and SK_Preferred feels non-intuitive. Is it because of the check:

case HK_SCALABLE:
    return (Val == SK_Disabled || Val == SK_Preferred);

that can only check Val to be 0 or 1?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
97–98

If I understand correctly, metadata hints take precedence over this flag unless the flag is set to off?
If this is the case and the default value of the flag (for now) is set to SK_Disabled, that means the metadata hints won't have an effect if the flag is not explicitly set to on or preferred, right?

paulwalker-arm added inline comments.May 12 2021, 9:48 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99–100

It looks weird to have two Kind's that have different definition/handling for Undefined.

99–108

Given the way the code is written, for example isScalableVectorizationPreferred() and the comments attached to the enum values I think the patch will be simpler if the enum names reflected their intent. For example:

Unspecified,
FixedOnly,
PreferFixed,
PreferScalable
sdesmalen updated this revision to Diff 345084.May 13 2021, 4:26 AM
sdesmalen marked 9 inline comments as done.
  • Moved isScalableVectorizationPreferred out of this patch, since it wasn't used.
  • Renamed enum values and made it signed again.
  • Fixed order of enum values (0 (disabled), 1 (fixed-width only), 2 (fixed-width preferred), 3 (scalable preferred), and remapped boolean i1 from metadata to from 1 -> 3.
  • Rebased patch.
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99–108

That makes the code a bit clearer indeed, so I've adopted your suggestion. I have left the external user-facing option the same though.

108

Yes the LLVM IR Metadata attribute is a boolean 0 or 1. I agree this is a bit non-intuitive, so I've fixed this now by passing the argument to Hint::validate as a reference, so that we can remap the value.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
97–98

Yes, that's right. This allows people to start experimenting with scalable vectors and using the pragma in their code, without it possibly breaking things unless they compile with the flag -scalable-vectorization=on|preferred.

A few stylistic things to consider that I'll not hold you to, but the change to validate is the main thing stopping me from accepting the patch.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
65–66

See comment on implementation.

99–111

Personally I think the following is prettier and more in keeping of the coding style used by other well documented enums:

enum ScalableForceKind {
  /// Not selected.
  SK_Unspecified = -1,   
  /// Disables vectorization with scalable vectors.
  SK_FixedWidthOnly = 0,
  /// Vectorize loops using scalable vectors or fixed-width
  /// vectors, but favor fixed-width vectors when the cost is
  /// inconclusive.
  SK_PreferFixedWidth = 1, 
  /// Vectorize loops using scalable vectors or fixed-width
  /// vectors, but favor scalable vectors when the cost-model is
  /// inconclusive. This is the default when the scalable.enable
  ///  hint is enabled through a pragma.
  SK_PreferScalable = 2
149–150

Perhaps nit picking but given the function says ExplicitlyEnabled, would it make more sense to explicitly compare against SK_PreferFixedWidth and SK_PreferScalable?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
60

What about Scalable vectorisation is disabled.

62–64

What about Scalable vectorisation available, but favor fixed-width vectorisation when the cost is inconclusive.?

66–67

What about Scalable vectorisation available and favored when the cost is inconclusive.?

73

I realise you've changed this in response to a previous comment but having a validation function change the value it's validating seems wrong. You could change the function name or perhaps return an optional or something but given the enum values have been renamed I wonder if the original comment has been nullified?

That's to say, whilst I agreed with the comment's original context, I don't see any loss of intuition if the renamed enum order became:

SK_FixedWidthOnly = 0
SK_PreferScalable = 1
SK_PreferFixedWidth = 2

and thus you would no longer need these changes.

sdesmalen marked 7 inline comments as done.
  • Addressed nits.
  • Changed validate to take argument by value, not by ref.
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
73

That's a good point, with the new names of the enum values I agree the change in order is equally intuitive.

paulwalker-arm accepted this revision.May 17 2021, 2:37 AM
paulwalker-arm added inline comments.
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
149–150

Now that you're not comparing against SK_Unspecified you probably don't need the (ScalableForceKind) casting. If you prefer to keep the casting then I suggest isScalableVectorizationDisabled() should also have it.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
60

Sorry my bad :) you probably want s/vectorisation/vectorization/

This revision is now accepted and ready to land.May 17 2021, 2:37 AM
fhahn added a subscriber: fhahn.May 17 2021, 2:40 AM
fhahn added inline comments.
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

can this be an enum class or is the code converting from ints to enum values somewhere?

paulwalker-arm added inline comments.May 17 2021, 3:46 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

I prefer that we remain consistent with the other Kind enums.

fhahn added inline comments.May 17 2021, 3:55 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

Using an enum class enforces stricter guarantees and allows the compiler the emit better warnings, which seem like tangible practical benefits. I'm not sure if consistency with other enums outweighs the practical benefits? It's quite common for parts of the codebase to evolve step-by-step.

Using an enum until the other enums are migrated leaves the door open for changes that make switching to an enum class harder.

sdesmalen added inline comments.May 17 2021, 4:01 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

There are several conversions between unsigned and the enum, so this requires adding casts to several places.

paulwalker-arm added inline comments.May 17 2021, 4:01 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

Sure I get that. I'm just saying I prefer all the enums associated with the LoopVectorizeHints class to be handled the same way.

fhahn added inline comments.May 17 2021, 4:06 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

There are several conversions between unsigned and the enum, so this requires adding casts to several places.

I missed that comment earlier. If there's the need for casting, it's a moot point anyways.

sdesmalen updated this revision to Diff 345822.May 17 2021, 4:13 AM
  • Addressed nits.
vkmr added inline comments.May 17 2021, 4:33 AM
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
60

Perhaps I am bike-shedding a bit much here (and I am not too sure about this), but the "on" and "off" terminology indicates binary choice, not making it readily clear that there is a possible third choice "preferred". Also reading -scalable-vectorization=on doesn't make it clear that it in fact prefers fixed vectorization.
May be something like "off", "possible" and "preferred" is clearer.
Thoughts?

sdesmalen added inline comments.May 17 2021, 9:55 AM
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
99

Np, thanks for the suggestion anyway!

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
60

It's a good question! From my perspective, having the flag set to 'on' kind of does what it promises, it enables scalable vectorization. It doesn't really suggest to what extend it enables scalable vectorization, which leaves us some room to play with what a good default is. Whereas 'preferred' suggests that scalable vectorization is the preferred way to vectorize loops, instead of fixed-width. I guess the interpretation of 'on' isn't too dissimilar to 'possible', but while the latter is more accurate for now, the former may be more accurate going forward. Unless you feel strongly about this, I have a slight preference to keep 'on' as this feels a bit more future-proof as a default.

I'm not too worried about having much confusion about this, because --help-hidden shows all three options with their meaning, and it's not really meant to be a user-facing option in the sense that end-users will see it, but rather an experimental option for now.

vkmr accepted this revision.May 18 2021, 4:00 AM

LGTM.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
60

I agree with the future-proof reasoning. I am fine with the way it is.

This revision was landed with ongoing or failed builds.May 19 2021, 2:42 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the feedback and suggestions @paulwalker-arm, @vkmr and @CarolineConcatto!