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

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882