[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.h:303
 
+  Optional<AMDGPU::IsaInfo::AMDGPUTargetID> TargetID;
+
----------------
arsenm wrote:
> t-tye wrote:
> > arsenm wrote:
> > > t-tye wrote:
> > > > arsenm wrote:
> > > > > kerbowa wrote:
> > > > > > arsenm wrote:
> > > > > > > Don't see why this would be Optional if the query just crashes if it's missing
> > > > > > It's not constructed until "initializeSubtargetDependencies" is called.
> > > > > It can just use an invalid target ID value then
> > > > 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.
> > > There's no reason to use Optional here since None is not a meaningful state. This is only an artifact of the initialization order
> > 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?
> It could just default to Unsupported or Any
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882