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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:962
+    dbgs() << "SRAMECC setting for subtarget: ";
+    switch (SramEcc) {
+    case (AMDGPUSubtarget::SramEccOn):
----------------
arsenm wrote:
> dbgs() << StringSwitch
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:71
+
+  enum SramEccSetting {
+    SramEccDefault = 1,
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:504
+
+  switch (AK) {
+  case GK_NONE:
----------------
arsenm wrote:
> t-tye wrote:
> > arsenm wrote:
> > > 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
> > 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?
> 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
> 
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**.
> 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882