[PATCH] D85882: [AMDGPU] Update subtarget features for new target ID support

[Austin Kerbow via Phabricator via llvm-commits]

kerbowa added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:91
   // unset everything else if it is disabled
+  TargetID.emplace(*this);
 
----------------
t-tye wrote:
> kerbowa wrote:
> > t-tye wrote:
> > > kerbowa wrote:
> > > > t-tye wrote:
> > > > > 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.
> > > > 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.
> > > 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?
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85882/new/

https://reviews.llvm.org/D85882