Page MenuHomePhabricator

[AMDGPU] Update subtarget features for new target ID support
AcceptedPublic

Authored by kerbowa on Aug 13 2020, 12:21 AM.

Details

Summary

We need to be able to differentiate between the four possible scenarios for
Xnack and SramEcc listed below:

Not Supported: The GPU has no support for Xnack/SramEcc
Any: Preference is unspecified. Use conservative settings that can run anywhere.
Off: Request support for Xnack/SramEcc Off
On: Request support for Xnack/SramEcc On

GCNSubtarget will track the four options based on the following criteria. If
the subtarget does not support xnack/sram-ecc we say the setting is
"NotSupported". If no subtarget features for xnack/sram-ecc are requested we
must support "Any" mode. If the subtarget features xnack/sram-ecc exist in the
feature string when initializing the subtarget, the settings are "On/Off".

Diff Detail

Event Timeline

kerbowa created this revision.Aug 13 2020, 12:21 AM
kerbowa requested review of this revision.Aug 13 2020, 12:21 AM
arsenm added inline comments.Aug 13 2020, 6:54 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
889–892

I was hoping this eliminated the string checks when I saw this was removed previously, but this is just moving the problem. This isn't really correct since the feature parsing doesn't care about any particular instance of the feature. e.g. +xnack,-xnack is valid and the last one wins. We really shouldn't have to guess at the intent of the feature string, and only rely on the properly parsed result

902–916

I don't think these should be fatal errors. SubtargetFeature is really missing ImpliesNot

935

I'm not sure we want debug printing in the subtarget constructor

936–950

StringSwitch

954–967

dbgs() << StringSwitch

968

Single quotes

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
65

The backend doesn't really know if the target feature was specified or why, so I don't see why we need to track the "default" concept. The default is just on if it's supported by the hardware like before

127

No reason to use virtual for this

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
500

Parsing shouldn't be necessary

504

Having to explicitly list all the targets that support this here is worse than having a subtarget feature indicating hardware support added to the processor definition

529

Parsing shouldn't be necessary

t-tye added inline comments.Aug 13 2020, 8:14 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
65

The backend does need to know as it may produce different code for the 3 settings. Currently that is not being done, but for sram-ecc there would be benefit in supporting different code gen for default and on so we can use D16 instructions and exploit that they zero the other bits when in sram-ecc on mode.

Also the ELF header encodes a distinction between default and non-default. This allows the loader to enforce compatibility between multiple loaded code objects with different settings.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
504

If subtarget features are intended to be simply an efficient way to represent the contents of the function attribute for target features then including if a target feature is supported does not seem to belong there. That is not a function attribute that makes sense for the user to specify as it is fundamental to the target.

Or is the subtarget a suitable place to include other properties in addition to the user specified function attributes? Or is there another place such target properties can be defined?

It does seem there are other helper functions that are very similar to this. So the comment here presumably applies to them too?

kerbowa added inline comments.Aug 13 2020, 8:39 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
889–892

Doesn't the parsing of the result have the same problem you are describing?

We had discussed adding a third target feature bit to track the "Default" setting. My concern was that since we are relying on target features to set these properties we shouldn't introduce more target features that break everything if they are modified. Do you know of any concept that can make some target features static and others dynamic?

Still, maybe adding the third feature are using the existing feature parsing is a better solution. Obviously the parsing here could be improved.

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
65

Yes, and the original plan was to keep the target features the same because the backend shouldn't care. The problem is that we need to enforce compatible features for every function, and in that sense "Default" and "On" have distinct meaning. The difference also matters when generating EFLAGs. We are adding a pass to check compatibility between target features, and "Default" is part of that equation.

For SramEcc "On", "Off", and "Default" will eventually generate different code.

It also makes sense to be consistent across the stack with the concepts in the new target ID.

127

I wasn't sure if this might ever be used with AMDGPUSubtarget or only GCNSubtarget.

arsenm added inline comments.Aug 13 2020, 10:24 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
65

This is the first I've heard of a pass to check compatibility. I hope this is at most a verifier tool

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
504

Subtarget features and function attributes are distinct things. The uses can blur since the subtarget features are present in a function attribute.

Subtarget features are a list of properties for a processor. Whether the hardware supports something is absolutely a distinct subtarget feature. You could make an argument for the "dynamic" setting for xnack/ecc should be an independent function attribute. However, since you can't really separate this for codegen purposes, I think it's a subtarget feature.

Really anywhere checking for specific GPUs is a problem. This pattern is just going to grow the number of places that need to be updated everytime a new target is added. I'd much rather consolidate as much as possible into the subtarget features attached to the processor definition

scott.linder added inline comments.Aug 13 2020, 10:59 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
954

I think this is the first instance where having distinct, weak types for each feature causes issues. I would prefer this be a function, e.g. raw_ostream& operator<<(raw_ostream&, TargetIDSetting) which really needs you to have one, strong type.

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
71

Not to distract from the more fundamental questions in the review, but can these all be enum class to get stronger type safety and namespacing? Then you can also drop the prefix on the variants, i.e.

enum class SramEccSetting { Default, On, Off, NotSupported };

Also, if we don't need to leverage the types to prevent e.g. passing an SramEccSetting to something expecting an XnackSetting (and I imagine this is the case, as generally everything will just be asking for the setting, not setting/passing it around), then I think we should just have one enum of the four variants that are common to all "TargetID Target Features", for example:

enum class TargetIDSetting { Default, On, Off, NotSupported };
TargetIDSetting getXnackSetting() const;
TargetIDSetting getSramEccSetting() const;

I think of this as more in line with having e.g. a new type for Meters or Miles. For each "unit" in the domain you should have a new type (specifically one which C++ actually type-checks), but you typically don't need/want BoatLengthMeters and CarLengthMeters because you sacrifice having things that deal with just Meters in a general way.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
504

I'm surprised there aren't other targets with the same issue?

Or is the idea that there should never be any really immutable target features? You should always be able to hack around on them with e.g. llc by using -mattr? It seems like https://llvm.org/doxygen/classllvm_1_1SubtargetFeatures.html#details hints at this with the paragraph (emphasis mine):

Features are encoded as a string of the form "+attr1,+attr2,-attr3,...,+attrN" A comma separates each feature from the next (all lowercase.) Each of the remaining features is prefixed with + or - indicating whether that feature should be enabled or disabled contrary to the cpu specification.

kerbowa updated this revision to Diff 285487.Aug 13 2020, 1:53 PM

Re-add DoesNotSupport features. Use enum class. No fatal error on setting mismatch.

arsenm added inline comments.Aug 13 2020, 4:05 PM
llvm/lib/Target/AMDGPU/AMDGPU.td
839

This shouldn't be on any target definitions

kerbowa added inline comments.Aug 13 2020, 4:21 PM
llvm/lib/Target/AMDGPU/AMDGPU.td
839

It's intentional in order to keep the current defaults. Targets with FeatureXNACK are ones that used to be default xnack on.

arsenm accepted this revision.Aug 13 2020, 5:58 PM

LGTM with nits, assuming the follow up change fixes some of the remaining hacks

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
900

Indentation

925

Single quotes

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
788

Early exit if !hasD16LoadStore and avoid querying getSramEccSetting?

This revision is now accepted and ready to land.Aug 13 2020, 5:58 PM
kerbowa updated this revision to Diff 285545.Aug 13 2020, 7:21 PM

Rename "Default" to "Any". Address comments.

kerbowa edited the summary of this revision. (Show Details)Aug 13 2020, 7:24 PM
kerbowa edited the summary of this revision. (Show Details)Aug 13 2020, 10:51 PM
kzhuravl added inline comments.Aug 14 2020, 7:34 AM
llvm/lib/Target/AMDGPU/AMDGPU.td
839

Is this correct? 908 supports xnack, but I think it does not have xnack enabled by default?

kerbowa added inline comments.Aug 14 2020, 8:50 AM
llvm/lib/Target/AMDGPU/AMDGPU.td
839

Before this patch the feature was omitted on the proc definition. Without FeatureDoseNotSupportXNACK the default was Xnack On. This slightly changes the elf test because of the gloablSTI interaction. The codegen is the same as before.

It all becomes irrelevant since we changing the default soon.