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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D41096#954710, @craig.topper wrote:

> 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.


Good point. We'll need a list somewhere (either here or in the frontend). knl/knm are the odd ones out here. We could always add -avx512-wide-vectors unless the CPU is knl/knm. Or we always do it here and adjust how we add the attribute in the frontend. What do you think?

> 
> 
>> 
>> 
>>> 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