This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

[AMDGPU] Update subtarget features for new target ID support

Support for XNACK and SRAMECC is not static on some GPUs. We must be able
to differentiate between different scenarios for these dynamic subtarget
features.

The possible settings are:

  • Unsupported: 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/SRAMECC we say the setting is
"Unsupported". If no subtarget features for XNACK/SRAMECC are requested we
must support "Any" mode. If the subtarget features XNACK/SRAMECC exist in the
feature string when initializing the subtarget, the settings are "On/Off".

The defaults are updated to be conservatively correct, meaning if no setting
for XNACK or SRAMECC is explicitly requested, defaults will be used which
generate code that can be run anywhere. This corresponds to the "Any" setting.

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
962–965

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

975–989

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

1008

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

1009–1023

StringSwitch

1027–1040

dbgs() << StringSwitch

1041

Single quotes

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
65 ↗(On Diff #285271)

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

141 ↗(On Diff #285271)

No reason to use virtual for this

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

Parsing shouldn't be necessary

597

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

622

Parsing shouldn't be necessary

t-tye added inline comments.Aug 13 2020, 8:14 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
65 ↗(On Diff #285271)

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
597

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
962–965

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 ↗(On Diff #285271)

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.

141 ↗(On Diff #285271)

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 ↗(On Diff #285271)

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
597

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
1027

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 ↗(On Diff #285271)

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
597

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
859

This shouldn't be on any target definitions

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

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
973

Indentation

998

Single quotes

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
788 ↗(On Diff #285487)

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
859

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
859

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.

kerbowa updated this revision to Diff 300902.Oct 26 2020, 11:16 PM

Update, rebase, and merge with change of default patch.

kerbowa edited the summary of this revision. (Show Details)Oct 26 2020, 11:17 PM
kerbowa updated this revision to Diff 308428.Nov 30 2020, 11:21 AM
kerbowa edited the summary of this revision. (Show Details)

Rebase. Move some utility functions to header.

arsenm added inline comments.Nov 30 2020, 11:23 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

Don't see why this would be Optional if the query just crashes if it's missing

kerbowa added inline comments.Nov 30 2020, 11:34 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

It's not constructed until "initializeSubtargetDependencies" is called.

arsenm added inline comments.Nov 30 2020, 11:34 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

It can just use an invalid target ID value then

t-tye added inline comments.Nov 30 2020, 1:39 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

But optional is a better way of indicating an object does not yet have a value than imposing that the value itself has a distinguished NULL value.

arsenm added inline comments.Nov 30 2020, 1:40 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

There's no reason to use Optional here since None is not a meaningful state. This is only an artifact of the initialization order

t-tye added inline comments.Nov 30 2020, 1:59 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

But wouldn't that mean there has to be a default constructor for AMDGPU::IsaInfo::AMDGPUTargetID? Is there such a default constructor? Is there a meaningful value to default construct to?

arsenm added inline comments.Nov 30 2020, 2:02 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

It could just default to Unsupported or Any

Any progress on this? Does it need another rebase?

thanks

cc @t-tye

t-tye added inline comments.Jan 17 2021, 10:20 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
303 ↗(On Diff #308428)

But it seems incorrect to default to a value that is not meaningful for the target. So it would need to be determinable whether the target did or did not support the features to know whether to use Unsupported or Any as the default setting. Is that possible at this point of the code, or is that figured out later when this field is given a non-None value?

kerbowa updated this revision to Diff 317241.Jan 17 2021, 11:55 AM

Don't use Optional type for TargetID.

t-tye added inline comments.Jan 17 2021, 12:31 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
91

Is moving constructing TargetID to the constructor of GCNSubtarget equivalent? Was it originally put in GCNSubtarget::initializeSubtargetDependencies because the base classes were modified between the construction of GCNSubtarget and calling its GCNSubtarget::initializeSubtargetDependencies? I know this was all rather contorted logic that made it hard to reason about.

kerbowa added inline comments.Jan 17 2021, 12:52 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
91

I think it is functionally equivalent. The static parts of the FeatureBits that determine whether a feature is supported are constructed early before initializeSubtargetDependencies is called. We can use this to have an initial value of 'Unsupported' or 'Any' for TargetID, and then do more processing based on the feature string when initializeSubtargetDependencies is called later.

t-tye added inline comments.Jan 17 2021, 1:05 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
91

So what does it mean to say "static parts"? Aren't all FeatureBits born equal? They are just bits that can be overridden by function attributes. Is there a function attribute that can change the "isXnackSupported" feature bit? If so what does that mean? Is that legal and should it be diagnosed as not allowed?

kerbowa added inline comments.Jan 17 2021, 1:54 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
91

Yes, I just meant the "static" feature bits before looking at function attributes or doing any other processing. It was always possible to do pathological things like disabling any features for any processor by using function attributes. I'm not aware of any target which tries to prevent this.

t-tye added inline comments.Jan 17 2021, 2:14 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
91

But what if the function attributes change the value of the is*Supported feature bits? You now have inconsistent state between the TargetId and the FeatureBits. But allowing the Is*Supported to be changed by function attributes seems wrong to allow in the first place.

kerbowa added inline comments.Jan 17 2021, 10:39 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
91

I do see your point, but you could say the same about any target feature, e.g. what if someone changes the value of FeatureFP64 with function attributes. You can expect to break things easily this way. No one should be modifying the Is*Supported feature the same way they shouldn't be modifying the FP64 feature.

arsenm added inline comments.Jan 19 2021, 11:07 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
169–173

I think having debug printing in the subtarget constructor would be a net annoyance

kerbowa added inline comments.Jan 21 2021, 11:18 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
169–173

We already had debug printouts in the constructor from ParseSubtargetFeatures. Do you think they should all be removed? Right now the output with '-debug-only=amdgpu-subtarget' looks like this, which seems helpful to me.

Features:+promote-alloca,+load-store-opt,+enable-ds128,+enable-prt-strict-null,
CPU:gfx900
TuneCPU:gfx900

xnack setting for subtarget: Any
sramecc setting for subtarget: Unsupported
arsenm added inline comments.Jan 22 2021, 1:10 PM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp
169–173

I forgot about those, so I guess it doesn't matter

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

Don't need .hasValue()

kerbowa updated this revision to Diff 318740.Jan 23 2021, 12:45 AM

Rebase. Address comments.

t-tye added inline comments.Jan 23 2021, 11:40 AM
llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
378 ↗(On Diff #317241)

Why did this get changed to DoesNotSupportSRAMECC? Same question for bool SupportsXNACK.

t-tye accepted this revision.Jan 26 2021, 11:10 AM

LGTM

This revision was landed with ongoing or failed builds.Jan 26 2021, 11:29 AM
This revision was automatically updated to reflect the committed changes.