[PATCH] D41096: [X86] Initial support for prefer-vector-width function attribute

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In https://reviews.llvm.org/D41096#954672, @hfinkel wrote:

> In https://reviews.llvm.org/D41096#954642, @craig.topper wrote:
>
> > Normally I would prefer additive features too.
> >
> > For the prefer-avx256 feature in this patch, I need the preference to only apply with -march=skylake-avx512/native and not with -mavx512f or with -march=knl. So I think that means it needs to be set in the default features in skylake-avx512 cpu definition. And the prefer-vector-width attribute needs to remove it if the user specifies a higher preference
> >
> > For the next patch that will deal with legalization. I think i still have to do something weird because I can't see the features implied by the CPU name string in getSubtargetImpl. So I can't just blindly enable a feature flag for 512-bit register support there. So I need to add something like "+requires512bitvectors" into the feature string based on the attribute, but the lack of a "required-vector-width" attribute implies we don't know for sure. So maybe its better to have "+no512bitvectors" if the attribute is present and set to value 256 or less. Then in X86ISelLowering we would enable 512 bit types with "hasAVX512 && !(no512bitvectors && prefer-avx256)"
> >
> > Do you see any better way?
>
>
> I would add a target feature, like:
>
>   def FeatureAVX512WideVectors : SubtargetFeature<"avx512-wide-vectors", "HasAVX512WideVectors", "true", "Enable use of 512-bit Vector Registers", [FeatureAVX512]>;
>   
>
> Then I'd add this feature, separately, to the relevant CPU definitions (KNLFeatures and SKXFeatures).
>
> Then, in getSubtargetImpl, if the CPU name is skylake-avx512 or skx, then look at the required-vector-width attribute. If it is present, and is <= 256, then add "-avx512-wide-vectors" to the feature string.
>
> I think gives us what we want with only additive features.


I don't really want a separate list of CPUs with this issue that we have to remember to update each time we add a CPU. Its likely "cannonlake" and "icelake" need this too so that's 4 strings we need to check already. At least in the td file we'll probably copy a CPU when we add the next one and we'll see the PreferAVX256 feature and make a conscious decision.   We already have several places that check a subtarget feature called isSLM() which is like checking CPU=="silvermont" and none of those places have been audited for goldmont. And https://reviews.llvm.org/D40282 which fixed another place that tried to do effectively a CPU string comparison and got it wrong.

> 
> 
>> and not with -mavx512f
> 
> Do you mean that you want -march=skylake -mavx512f should effectively set a preference for 512-bit vectors? If so, I think we can handle that in the frontend (by just setting the preference there appropriately).

No I was only referring to a -mavx512f with no -march case. If a user has specified -march that preference should stay.


https://reviews.llvm.org/D41096