[AMDGPU] Update subtarget features for new target ID support

[kerbowa]

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.

[arsenm]

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]

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]

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]

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]

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]

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

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPU.td

859

This shouldn't be on any target definitions

[kerbowa]

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]

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?

[kerbowa]

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

[kzhuravl]

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]

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.

[kzhuravl]

LGTM

[kerbowa]

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

[kerbowa]

Rebase. Move some utility functions to header.

[arsenm]

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]

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h

303 ↗

(On Diff #308428)

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

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h

303 ↗

(On Diff #308428)

It can just use an invalid target ID value then

[t-tye]

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]

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]

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]

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h

303 ↗

(On Diff #308428)

It could just default to Unsupported or Any

[kzhuravl]

Any progress on this? Does it need another rebase?

thanks

cc @t-tye

[kerbowa]

Rebase.

[t-tye]

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]

Don't use Optional type for TargetID.

[t-tye]

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]

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]

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]

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]

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]

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]

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp

169–173

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

[kerbowa]

Rebase after 560d7e04113bf.

[kerbowa]

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]

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]

Rebase. Address comments.

[t-tye]

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h

378 ↗

(On Diff #317241)

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

[t-tye]

LGTM

[kerbowa]

Rebase.

[kzhuravl]

LGTM

llvm/lib/Target/AMDGPU/AMDGPU.td

 >;
 
 def FeatureFmaMixInsts : SubtargetFeature<"fma-mix-insts",
   "HasFmaMixInsts",
   "true",
   "Has v_fma_mix_f32, v_fma_mixlo_f16, v_fma_mixhi_f16 instructions"
 >;
 
-def FeatureDoesNotSupportXNACK : SubtargetFeature<"no-xnack-support",
-  "DoesNotSupportXNACK",
+def FeatureSupportsXNACK : SubtargetFeature<"xnack-support",
+  "SupportsXNACK",
   "true",
-  "Hardware does not support XNACK"
+  "Hardware supports XNACK"
 >;
 
 // XNACK is disabled if SH_MEM_CONFIG.ADDRESS_MODE = GPUVM on chips that support
 // XNACK. The current default kernel driver setting is:
 // - graphics ring: XNACK disabled
 // - compute ring: XNACK enabled
 //
 // If XNACK is enabled, the VMEM latency can be worse.

 def FeatureAtomicFaddInsts : SubtargetFeature<"atomic-fadd-insts",
   "HasAtomicFaddInsts",
   "true",
   "Has buffer_atomic_add_f32, buffer_atomic_pk_add_f16, global_atomic_add_f32, "
   "global_atomic_pk_add_f16 instructions",
   [FeatureFlatGlobalInsts]
 >;
 
-def FeatureDoesNotSupportSRAMECC : SubtargetFeature<"no-sram-ecc-support",
-  "DoesNotSupportSRAMECC",
+def FeatureSupportsSRAMECC : SubtargetFeature<"sramecc-support",
+  "SupportsSRAMECC",
   "true",
-  "Hardware does not support SRAM ECC"
+  "Hardware supports SRAMECC"
 >;
 
-def FeatureSRAMECC : SubtargetFeature<"sram-ecc",
+def FeatureSRAMECC : SubtargetFeature<"sramecc",
   "EnableSRAMECC",
   "true",
-  "Enable SRAM ECC"
+  "Enable SRAMECC"
 >;
 
 def FeatureNoSdstCMPX : SubtargetFeature<"no-sdst-cmpx",
   "HasNoSdstCMPX",
   "true",
   "V_CMPX does not write VCC/SGPR in addition to EXEC"
 >;
 
class GCNSubtargetFeatureGeneration <string Value,
                                      list<SubtargetFeature> Implies> :
         SubtargetFeatureGeneration <Value, FeatureName, "GCNSubtarget", Implies>;
 
 def FeatureSouthernIslands : GCNSubtargetFeatureGeneration<"SOUTHERN_ISLANDS",
     "southern-islands",
   [FeatureFP64, FeatureLocalMemorySize32768, FeatureMIMG_R128,
   FeatureWavefrontSize64, FeatureSMemTimeInst, FeatureMadMacF32Insts,
   FeatureDsSrc2Insts, FeatureLDSBankCount32, FeatureMovrel,
-  FeatureTrigReducedRange, FeatureDoesNotSupportSRAMECC,
-  FeatureDoesNotSupportXNACK]
+  FeatureTrigReducedRange]
 >;
 
 def FeatureSeaIslands : GCNSubtargetFeatureGeneration<"SEA_ISLANDS",
     "sea-islands",
   [FeatureFP64, FeatureLocalMemorySize65536, FeatureMIMG_R128,
   FeatureWavefrontSize64, FeatureFlatAddressSpace,
   FeatureCIInsts, FeatureMovrel, FeatureTrigReducedRange,
   FeatureGFX7GFX8GFX9Insts, FeatureSMemTimeInst, FeatureMadMacF32Insts,
-  FeatureDsSrc2Insts, FeatureDoesNotSupportSRAMECC,
-  FeatureUnalignedBufferAccess]
+  FeatureDsSrc2Insts, FeatureUnalignedBufferAccess]
 >;
 
 def FeatureVolcanicIslands : GCNSubtargetFeatureGeneration<"VOLCANIC_ISLANDS",
   "volcanic-islands",
   [FeatureFP64, FeatureLocalMemorySize65536, FeatureMIMG_R128,
    FeatureWavefrontSize64, FeatureFlatAddressSpace,
    FeatureGCN3Encoding, FeatureCIInsts, Feature16BitInsts,
    FeatureSMemRealTime, FeatureVGPRIndexMode, FeatureMovrel,
    FeatureScalarStores, FeatureInv2PiInlineImm,
    FeatureSDWA, FeatureSDWAOutModsVOPC, FeatureSDWAMac, FeatureDPP,
    FeatureIntClamp, FeatureTrigReducedRange, FeatureGFX8Insts,
    FeatureGFX7GFX8GFX9Insts, FeatureSMemTimeInst, FeatureMadMacF32Insts,
-   FeatureDsSrc2Insts, FeatureDoesNotSupportSRAMECC, FeatureFastDenormalF32,
-   FeatureUnalignedBufferAccess
-  ]
+   FeatureDsSrc2Insts, FeatureFastDenormalF32, FeatureUnalignedBufferAccess]
 >;
 
 def FeatureGFX9 : GCNSubtargetFeatureGeneration<"GFX9",
   "gfx9",
   [FeatureFP64, FeatureLocalMemorySize65536,
    FeatureWavefrontSize64, FeatureFlatAddressSpace,
    FeatureGCN3Encoding, FeatureCIInsts, Feature16BitInsts,
    FeatureSMemRealTime, FeatureScalarStores, FeatureInv2PiInlineImm,
    FeatureApertureRegs, FeatureGFX9Insts, FeatureVOP3P, FeatureVGPRIndexMode,
    FeatureFastFMAF32, FeatureDPP, FeatureIntClamp,
    FeatureSDWA, FeatureSDWAOmod, FeatureSDWAScalar, FeatureSDWASdst,
    FeatureFlatInstOffsets, FeatureFlatGlobalInsts, FeatureFlatScratchInsts,
    FeatureAddNoCarryInsts, FeatureGFX8Insts, FeatureGFX7GFX8GFX9Insts,
    FeatureScalarFlatScratchInsts, FeatureScalarAtomics, FeatureR128A16,
    FeatureSMemTimeInst, FeatureMadMacF32Insts, FeatureDsSrc2Insts,
-   FeatureFastDenormalF32, FeatureUnalignedBufferAccess,
-   FeatureUnalignedDSAccess
-  ]
+   FeatureFastDenormalF32, FeatureUnalignedBufferAccess, FeatureUnalignedDSAccess,
+   FeatureSupportsXNACK]
 >;
 
 def FeatureGFX10 : GCNSubtargetFeatureGeneration<"GFX10",
   "gfx10",
   [FeatureFP64, FeatureLocalMemorySize65536, FeatureMIMG_R128,
    FeatureFlatAddressSpace,
    FeatureCIInsts, Feature16BitInsts,
    FeatureSMemRealTime, FeatureInv2PiInlineImm,
    FeatureApertureRegs, FeatureGFX9Insts, FeatureGFX10Insts, FeatureVOP3P,
    FeatureMovrel, FeatureFastFMAF32, FeatureDPP, FeatureIntClamp,
    FeatureSDWA, FeatureSDWAOmod, FeatureSDWAScalar, FeatureSDWASdst,
    FeatureFlatInstOffsets, FeatureFlatGlobalInsts, FeatureFlatScratchInsts,
    FeatureAddNoCarryInsts, FeatureFmaMixInsts, FeatureGFX8Insts,
    FeatureNoSdstCMPX, FeatureVscnt, FeatureRegisterBanking,
    FeatureVOP3Literal, FeatureDPP8,
-   FeatureNoDataDepHazard, FeaturePkFmacF16Inst, FeatureDoesNotSupportSRAMECC,
+   FeatureNoDataDepHazard, FeaturePkFmacF16Inst,
    FeatureGFX10A16, FeatureFastDenormalF32, FeatureG16,
    FeatureUnalignedBufferAccess, FeatureUnalignedDSAccess
   ]
 >;
 
 class FeatureSet<list<SubtargetFeature> Features_> {
   list<SubtargetFeature> Features = Features_;
 }
 
 def FeatureISAVersion6_0_0 : FeatureSet<[FeatureSouthernIslands,
    FeatureFastFMAF32,
    HalfRate64Ops,
-   FeatureLDSBankCount32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount32]>;
 
 def FeatureISAVersion6_0_1 : FeatureSet<
   [FeatureSouthernIslands,
-   FeatureLDSBankCount32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount32]>;
 
 def FeatureISAVersion6_0_2 : FeatureSet<
   [FeatureSouthernIslands,
-   FeatureLDSBankCount32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount32]>;
 
 def FeatureISAVersion7_0_0 : FeatureSet<
   [FeatureSeaIslands,
-   FeatureLDSBankCount32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount32]>;
 
 def FeatureISAVersion7_0_1 : FeatureSet<
   [FeatureSeaIslands,
    HalfRate64Ops,
    FeatureLDSBankCount32,
-   FeatureFastFMAF32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureFastFMAF32]>;
 
 def FeatureISAVersion7_0_2 : FeatureSet<
   [FeatureSeaIslands,
    FeatureLDSBankCount16,
-   FeatureFastFMAF32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureFastFMAF32]>;
 
 def FeatureISAVersion7_0_3 : FeatureSet<
   [FeatureSeaIslands,
-   FeatureLDSBankCount16,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount16]>;
 
 def FeatureISAVersion7_0_4 : FeatureSet<
   [FeatureSeaIslands,
-   FeatureLDSBankCount32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount32]>;
 
 def FeatureISAVersion7_0_5 : FeatureSet<
   [FeatureSeaIslands,
-   FeatureLDSBankCount16,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureLDSBankCount16]>;
 
 def FeatureISAVersion8_0_1 : FeatureSet<
   [FeatureVolcanicIslands,
    FeatureFastFMAF32,
    HalfRate64Ops,
    FeatureLDSBankCount32,
-   FeatureXNACK,
+   FeatureSupportsXNACK,
    FeatureUnpackedD16VMem]>;
 
 def FeatureISAVersion8_0_2 : FeatureSet<
   [FeatureVolcanicIslands,
    FeatureLDSBankCount32,
    FeatureSGPRInitBug,
-   FeatureUnpackedD16VMem,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureUnpackedD16VMem]>;
 
 def FeatureISAVersion8_0_3 : FeatureSet<
   [FeatureVolcanicIslands,
    FeatureLDSBankCount32,
-   FeatureUnpackedD16VMem,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureUnpackedD16VMem]>;
 
 def FeatureISAVersion8_0_5 : FeatureSet<
   [FeatureVolcanicIslands,
    FeatureLDSBankCount32,
    FeatureSGPRInitBug,
-   FeatureUnpackedD16VMem,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureUnpackedD16VMem]>;
 
 def FeatureISAVersion8_1_0 : FeatureSet<
   [FeatureVolcanicIslands,
    FeatureLDSBankCount16,
-   FeatureXNACK,
+   FeatureSupportsXNACK,
    FeatureImageStoreD16Bug,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_0 : FeatureSet<
   [FeatureGFX9,
    FeatureMadMixInsts,
    FeatureLDSBankCount32,
-   FeatureDoesNotSupportXNACK,
-   FeatureDoesNotSupportSRAMECC,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_2 : FeatureSet<
   [FeatureGFX9,
    FeatureMadMixInsts,
    FeatureLDSBankCount32,
-   FeatureXNACK,
-   FeatureDoesNotSupportSRAMECC,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_4 : FeatureSet<
   [FeatureGFX9,
    FeatureLDSBankCount32,
    FeatureFmaMixInsts,
-   FeatureDoesNotSupportXNACK,
-   FeatureDoesNotSupportSRAMECC,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_6 : FeatureSet<
   [FeatureGFX9,
    HalfRate64Ops,
    FeatureFmaMixInsts,
    FeatureLDSBankCount32,
    FeatureDLInsts,
    FeatureDot1Insts,
    FeatureDot2Insts,
-   FeatureDoesNotSupportXNACK,
+   FeatureSupportsSRAMECC,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_8 : FeatureSet<
   [FeatureGFX9,
    HalfRate64Ops,
    FeatureFmaMixInsts,
    FeatureLDSBankCount32,
    FeatureDLInsts,
    FeatureDot1Insts,
    FeatureDot2Insts,
    FeatureDot3Insts,
    FeatureDot4Insts,
    FeatureDot5Insts,
    FeatureDot6Insts,
    FeatureMAIInsts,
    FeaturePkFmacF16Inst,
    FeatureAtomicFaddInsts,
-   FeatureSRAMECC,
+   FeatureSupportsSRAMECC,
    FeatureMFMAInlineLiteralBug,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_9 : FeatureSet<
   [FeatureGFX9,
    FeatureMadMixInsts,
    FeatureLDSBankCount32,
-   FeatureXNACK,
    FeatureImageGather4D16Bug]>;
 
 def FeatureISAVersion9_0_C : FeatureSet<
   [FeatureGFX9,
    FeatureMadMixInsts,
    FeatureLDSBankCount32,
    FeatureXNACK,
    FeatureImageGather4D16Bug]>;
!listconcat(FeatureGroup.GFX10_1_Bugs,
      FeatureScalarStores,
      FeatureScalarAtomics,
      FeatureScalarFlatScratchInsts,
      FeatureGetWaveIdInst,
      FeatureSMemTimeInst,
      FeatureMadMacF32Insts,
      FeatureDsSrc2Insts,
      FeatureLdsMisalignedBug,
-     FeatureDoesNotSupportXNACK])>;
+     FeatureSupportsXNACK])>;
 
 def FeatureISAVersion10_1_1 : FeatureSet<
   !listconcat(FeatureGroup.GFX10_1_Bugs,
     [FeatureGFX10,
      FeatureLDSBankCount32,
      FeatureDLInsts,
      FeatureDot1Insts,
      FeatureDot2Insts,
      FeatureDot5Insts,
      FeatureDot6Insts,
      FeatureNSAEncoding,
      FeatureWavefrontSize32,
      FeatureScalarStores,
      FeatureScalarAtomics,
      FeatureScalarFlatScratchInsts,
      FeatureGetWaveIdInst,
      FeatureSMemTimeInst,
      FeatureMadMacF32Insts,
      FeatureDsSrc2Insts,
      FeatureLdsMisalignedBug,
-     FeatureDoesNotSupportXNACK])>;
+     FeatureSupportsXNACK])>;
 
 def FeatureISAVersion10_1_2 : FeatureSet<
   !listconcat(FeatureGroup.GFX10_1_Bugs,
     [FeatureGFX10,
      FeatureLDSBankCount32,
      FeatureDLInsts,
      FeatureDot1Insts,
      FeatureDot2Insts,
      FeatureDot5Insts,
      FeatureDot6Insts,
      FeatureNSAEncoding,
      FeatureWavefrontSize32,
      FeatureScalarStores,
      FeatureScalarAtomics,
      FeatureScalarFlatScratchInsts,
      FeatureGetWaveIdInst,
      FeatureSMemTimeInst,
      FeatureMadMacF32Insts,
      FeatureDsSrc2Insts,
      FeatureLdsMisalignedBug,
-     FeatureDoesNotSupportXNACK])>;
+     FeatureSupportsXNACK])>;
 
 def FeatureISAVersion10_3_0 : FeatureSet<
   [FeatureGFX10,
    FeatureGFX10_BEncoding,
    FeatureGFX10_3Insts,
    FeatureLDSBankCount32,
    FeatureDLInsts,
    FeatureDot1Insts,
    FeatureDot2Insts,
    FeatureDot5Insts,
    FeatureDot6Insts,
    FeatureNSAEncoding,
-   FeatureWavefrontSize32,
-   FeatureDoesNotSupportXNACK]>;
+   FeatureWavefrontSize32]>;
 
 //===----------------------------------------------------------------------===//
 
 def AMDGPUInstrInfo : InstrInfo {
   let guessInstructionProperties = 1;
   let noNamedPositionallyEncodedOperands = 1;
 }
 

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp

 #include "AMDGPUSubtarget.h"
 #include "AMDGPU.h"
 #include "AMDGPUCallLowering.h"
 #include "AMDGPUInstructionSelector.h"
 #include "AMDGPULegalizerInfo.h"
 #include "AMDGPURegisterBankInfo.h"
 #include "AMDGPUTargetMachine.h"
 #include "SIMachineFunctionInfo.h"
+#include "Utils/AMDGPUBaseInfo.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/CodeGen/GlobalISel/InlineAsmLowering.h"
 #include "llvm/CodeGen/MachineScheduler.h"
 #include "llvm/CodeGen/TargetFrameLowering.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
 #include "llvm/IR/IntrinsicsR600.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/MC/MCSubtargetInfo.h"
GCNSubtarget::initializeSubtargetDependencies(const Triple &TT,
   // Determine default and user-specified characteristics
   //
   // We want to be able to turn these off, but making this a subtarget feature
   // for SI has the unhelpful behavior that it unsets everything else if you
   // disable it.
   //
   // Similarly we want enable-prt-strict-null to be on by default and not to
   // unset everything else if it is disabled
 
-  // Assuming ECC is enabled is the conservative default.
-  SmallString<256> FullFS("+promote-alloca,+load-store-opt,+enable-ds128,+sram-ecc,+xnack,");
+  SmallString<256> FullFS("+promote-alloca,+load-store-opt,+enable-ds128,");
 
   // Turn on features that HSA ABI requires. Also turn on FlatForGlobal by default
   if (isAmdHsaOS())
     FullFS += "+flat-for-global,+unaligned-access-mode,+trap-handler,";
 
   FullFS += "+enable-prt-strict-null,"; // This is overridden by a disable in FS
 
   // Disable mutually exclusive bits.
GCNSubtarget::initializeSubtargetDependencies(const Triple &TT,
   }
 
   // Don't crash on invalid devices.
   if (WavefrontSizeLog2 == 0)
     WavefrontSizeLog2 = 5;
 
   HasFminFmaxLegacy = getGeneration() < AMDGPUSubtarget::VOLCANIC_ISLANDS;
 
-  // Disable XNACK on targets where it is not enabled by default unless it is
-  // explicitly requested.
-  if (!FS.contains("+xnack") && DoesNotSupportXNACK && EnableXNACK) {
-    ToggleFeature(AMDGPU::FeatureXNACK);
-    EnableXNACK = false;
-  }
+  TargetID.setTargetIDFromFeaturesString(FS);
 
-  // ECC is on by default, but turn it off if the hardware doesn't support it
-  // anyway. This matters for the gfx9 targets with d16 loads, but don't support
-  // ECC.
-  if (DoesNotSupportSRAMECC && EnableSRAMECC) {
-    ToggleFeature(AMDGPU::FeatureSRAMECC);
-    EnableSRAMECC = false;
-  }
+  LLVM_DEBUG(dbgs() << "xnack setting for subtarget: "
+                    << TargetID.getXnackSetting() << '\n');
+  LLVM_DEBUG(dbgs() << "sramecc setting for subtarget: "
+                    << TargetID.getSramEccSetting() << '\n');
 
   return *this;
 }
 
 AMDGPUSubtarget::AMDGPUSubtarget(const Triple &TT) :
   TargetTriple(TT),
   Has16BitInsts(false),
   HasMadMixInsts(false),
   HasMadMacF32Insts(false),
AMDGPUSubtarget::AMDGPUSubtarget(const Triple &TT) :
   WavefrontSizeLog2(0)
   { }
 
 GCNSubtarget::GCNSubtarget(const Triple &TT, StringRef GPU, StringRef FS,
                            const GCNTargetMachine &TM) :
     AMDGPUGenSubtargetInfo(TT, GPU, /*TuneCPU*/ GPU, FS),
     AMDGPUSubtarget(TT),
     TargetTriple(TT),
+    TargetID(*this),
     Gen(INVALID),
     InstrItins(getInstrItineraryForCPU(GPU)),
     LDSBankCount(0),
     MaxPrivateElementSize(0),
 
     FastFMAF32(false),
     FastDenormalF32(false),
     HalfRate64Ops(false),
 
     FlatForGlobal(false),
     AutoWaitcntBeforeBarrier(false),
     UnalignedScratchAccess(false),
     UnalignedAccessMode(false),
 
     HasApertureRegs(false),
+    SupportsXNACK(false),
     EnableXNACK(false),
-    DoesNotSupportXNACK(false),
     EnableCuMode(false),
     TrapHandler(false),
 
     EnableLoadStoreOpt(false),
     EnableUnsafeDSOffsetFolding(false),
     EnableSIScheduler(false),
     EnableDS128(false),
     EnablePRTStrictNull(false),
GCNSubtarget::GCNSubtarget(const Triple &TT, StringRef GPU, StringRef FS,
     HasDot2Insts(false),
     HasDot3Insts(false),
     HasDot4Insts(false),
     HasDot5Insts(false),
     HasDot6Insts(false),
     HasMAIInsts(false),
     HasPkFmacF16Inst(false),
     HasAtomicFaddInsts(false),
+    SupportsSRAMECC(false),
     EnableSRAMECC(false),
-    DoesNotSupportSRAMECC(false),
     HasNoSdstCMPX(false),
     HasVscnt(false),
     HasGetWaveIdInst(false),
     HasSMemTimeInst(false),
     HasRegisterBanking(false),
     HasVOP3Literal(false),
     HasNoDataDepHazard(false),
     FlatAddressSpace(false),

 } // namespace
 
 void GCNSubtarget::getPostRAMutations(
     std::vector<std::unique_ptr<ScheduleDAGMutation>> &Mutations) const {
   Mutations.push_back(std::make_unique<FillMFMAShadowMutation>(&InstrInfo));
 }
 
 const AMDGPUSubtarget &AMDGPUSubtarget::get(const MachineFunction &MF) {
   if (MF.getTarget().getTargetTriple().getArch() == Triple::amdgcn)
     return static_cast<const AMDGPUSubtarget&>(MF.getSubtarget<GCNSubtarget>());
   else
     return static_cast<const AMDGPUSubtarget&>(MF.getSubtarget<R600Subtarget>());
 }
 
 const AMDGPUSubtarget &AMDGPUSubtarget::get(const TargetMachine &TM, const Function &F) {
   if (TM.getTargetTriple().getArch() == Triple::amdgcn)
     return static_cast<const AMDGPUSubtarget&>(TM.getSubtarget<GCNSubtarget>(F));
   else
     return static_cast<const AMDGPUSubtarget&>(TM.getSubtarget<R600Subtarget>(F));
 }

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

 // header files, so we need to wrap it in a function that takes unsigned
 // instead.
 int getMCOpcode(uint16_t Opcode, unsigned Gen) {
   return getMCOpcodeGen(Opcode, static_cast<Subtarget>(Gen));
 }
 
 namespace IsaInfo {
 
+AMDGPUTargetID::AMDGPUTargetID(const MCSubtargetInfo &STI)
+    : XnackSetting(TargetIDSetting::Any), SramEccSetting(TargetIDSetting::Any) {
+  if (!STI.getFeatureBits().test(FeatureSupportsXNACK))
+    XnackSetting = TargetIDSetting::Unsupported;
+  if (!STI.getFeatureBits().test(FeatureSupportsSRAMECC))
+    SramEccSetting = TargetIDSetting::Unsupported;
+}
+
+void AMDGPUTargetID::setTargetIDFromFeaturesString(StringRef FS) {
+  // Check if xnack or sramecc is explicitly enabled or disabled.  In the
+  // absence of the target features we assume we must generate code that can run
+  // in any environment.
+  SubtargetFeatures Features(FS);
+  Optional<bool> XnackRequested;
+  Optional<bool> SramEccRequested;
+
+  for (const std::string &Feature : Features.getFeatures()) {
+    if (Feature == "+xnack")
+      XnackRequested = true;
+    else if (Feature == "-xnack")
+      XnackRequested = false;
+    else if (Feature == "+sramecc")
+      SramEccRequested = true;
+    else if (Feature == "-sramecc")
+      SramEccRequested = false;
+  }
+
+  bool XnackSupported = isXnackSupported();
+  bool SramEccSupported = isSramEccSupported();
+
+  if (XnackRequested) {
+    if (XnackSupported) {
+      XnackSetting =
+          *XnackRequested ? TargetIDSetting::On : TargetIDSetting::Off;
+    } else {
+      // If a specific xnack setting was requested and this GPU does not support
+      // xnack emit a warning. Setting will remain set to "Unsupported".
+      if (*XnackRequested) {
+        errs() << "warning: xnack 'On' was requested for a processor that does "
+                  "not support it!\n";
+      } else {
+        errs() << "warning: xnack 'Off' was requested for a processor that "
+                  "does not support it!\n";
+      }
+    }
+  }
+
+  if (SramEccRequested) {
+    if (SramEccSupported) {
+      SramEccSetting =
+          *SramEccRequested ? TargetIDSetting::On : TargetIDSetting::Off;
+    } else {
+      // If a specific sramecc setting was requested and this GPU does not
+      // support sramecc emit a warning. Setting will remain set to
+      // "Unsupported".
+      if (*SramEccRequested) {
+        errs() << "warning: sramecc 'On' was requested for a processor that "
+                  "does not support it!\n";
+      } else {
+        errs() << "warning: sramecc 'Off' was requested for a processor that "
+                  "does not support it!\n";
+      }
+    }
+  }
+}
+
+static TargetIDSetting
+getTargetIDSettingFromFeatureString(StringRef FeatureString) {
+  if (FeatureString.endswith("-"))
+    return TargetIDSetting::Off;
+  if (FeatureString.endswith("+"))
+    return TargetIDSetting::On;
+
+  llvm_unreachable("Malformed feature string");
+}
+
+void AMDGPUTargetID::setTargetIDFromTargetIDStream(StringRef TargetID) {
+  SmallVector<StringRef, 3> TargetIDSplit;
+  TargetID.split(TargetIDSplit, ':');
+
+  for (const auto &FeatureString : TargetIDSplit) {
+    if (FeatureString.startswith("xnack"))
+      XnackSetting = getTargetIDSettingFromFeatureString(FeatureString);
+    if (FeatureString.startswith("sramecc"))
+      SramEccSetting = getTargetIDSettingFromFeatureString(FeatureString);
+  }
+}
+
 void streamIsaVersion(const MCSubtargetInfo *STI, raw_ostream &Stream) {
   auto TargetTriple = STI->getTargetTriple();
   auto Version = getIsaVersion(STI->getCPU());
 
   Stream << TargetTriple.getArchName() << '-'
          << TargetTriple.getVendorName() << '-'
          << TargetTriple.getOSName() << '-'
          << TargetTriple.getEnvironmentName() << '-'
          << "gfx"
          << Version.Major
          << Version.Minor
          << Version.Stepping;
 
   if (hasXNACK(*STI))
     Stream << "+xnack";
   if (hasSRAMECC(*STI))
-    Stream << "+sram-ecc";
+    Stream << "+sramecc";
 
   Stream.flush();
 }
 
 unsigned getWavefrontSize(const MCSubtargetInfo *STI) {
   if (STI->getFeatureBits().test(FeatureWavefrontSize16))
     return 16;
   if (STI->getFeatureBits().test(FeatureWavefrontSize32))
unsigned getNumVGPRBlocks(const MCSubtargetInfo *STI, unsigned NumVGPRs,
                           Optional<bool> EnableWavefrontSize32) {
   NumVGPRs = alignTo(std::max(1u, NumVGPRs),
                      getVGPREncodingGranule(STI, EnableWavefrontSize32));
   // VGPRBlocks is actual number of VGPR blocks minus 1.
   return NumVGPRs / getVGPREncodingGranule(STI, EnableWavefrontSize32) - 1;
 }
 
 } // end namespace IsaInfo
 
 void initDefaultAMDKernelCodeT(amd_kernel_code_t &Header,
                                const MCSubtargetInfo *STI) {
   IsaVersion Version = getIsaVersion(STI->getCPU());
 
   memset(&Header, 0, sizeof(Header));
 
   Header.amd_kernel_code_version_major = 1;
   Header.amd_kernel_code_version_minor = 2;
   Header.amd_machine_kind = 1; // AMD_MACHINE_KIND_AMDGPU
   Header.amd_machine_version_major = Version.Major;
   Header.amd_machine_version_minor = Version.Minor;
   Header.amd_machine_version_stepping = Version.Stepping;
   Header.kernel_code_entry_byte_offset = sizeof(Header);
   Header.wavefront_size = 6;
 
   // If the code object does not support indirect functions, then the value must
   // be 0xffffffff.
   Header.call_convention = -1;
 
   // These alignment values are specified in powers of two, so alignment =
   // 2^n.  The minimum alignment is 2^4 = 16.
   Header.kernarg_segment_alignment = 4;
   Header.group_segment_alignment = 4;
   Header.private_segment_alignment = 4;
 
   if (Version.Major >= 10) {
     if (STI->getFeatureBits().test(FeatureWavefrontSize32)) {
       Header.wavefront_size = 5;
       Header.code_properties |= AMD_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32;
     }
     Header.compute_pgm_resource_registers |=
       S_00B848_WGP_MODE(STI->getFeatureBits().test(FeatureCuMode) ? 0 : 1) |
       S_00B848_MEM_ORDERED(1);
   }
 }
 
 amdhsa::kernel_descriptor_t getDefaultAmdhsaKernelDescriptor(

 
 const GcnBufferFormatInfo *getGcnBufferFormatInfo(uint8_t Format,
                                                   const MCSubtargetInfo &STI) {
   return isGFX10Plus(STI) ? getGfx10PlusBufferFormatInfo(Format)
                           : getGfx9BufferFormatInfo(Format);
 }
 
 } // namespace AMDGPU
+
+raw_ostream &operator<<(raw_ostream &OS,
+                        const AMDGPU::IsaInfo::TargetIDSetting S) {
+  switch (S) {
+  case (AMDGPU::IsaInfo::TargetIDSetting::Unsupported):
+    OS << "Unsupported";
+    break;
+  case (AMDGPU::IsaInfo::TargetIDSetting::Any):
+    OS << "Any";
+    break;
+  case (AMDGPU::IsaInfo::TargetIDSetting::Off):
+    OS << "Off";
+    break;
+  case (AMDGPU::IsaInfo::TargetIDSetting::On):
+    OS << "On";
+    break;
+  }
+  return OS;
+}
+
 } // namespace llvm