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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:303
 
+  Optional<AMDGPU::IsaInfo::AMDGPUTargetID> TargetID;
+
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882