[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:897-900
+  bool XnackOnSupportRequested = FS.contains("+xnack");
+  bool SramEccOnSupportRequested = FS.contains("+sram-ecc");
+  bool XnackOffSupportRequested = FS.contains("-xnack");
+  bool SramEccOffSupportRequested = FS.contains("-sram-ecc");
----------------
arsenm wrote:
> I was hoping this eliminated the string checks when I saw this was removed previously, but this is just moving the problem. This isn't really correct since the feature parsing doesn't care about any particular instance of the feature. e.g. +xnack,-xnack is valid and the last one wins. We really shouldn't have to guess at the intent of the feature string, and only rely on the properly parsed result
Doesn't the parsing of the result have the same problem you are describing?

We had discussed adding a third target feature bit to track the "Default" setting. My concern was that since we are relying on target features to set these properties we shouldn't introduce more target features that break everything if they are modified. Do you know of any concept that can make some target features static and others dynamic?

Still, maybe adding the third feature are using the existing feature parsing is a better solution. Obviously the parsing here could be improved.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:65
+  enum XnackSetting {
+    XnackDefault = 1,
+    XnackOn = 2,
----------------
t-tye wrote:
> arsenm wrote:
> > The backend doesn't really know if the target feature was specified or why, so I don't see why we need to track the "default" concept. The default is just on if it's supported by the hardware like before
> The backend does need to know as it may produce different code for the 3 settings. Currently that is not being done, but for sram-ecc there would be benefit in supporting different code gen for default and on so we can use D16 instructions and exploit that they zero the other bits when in sram-ecc on mode.
> 
> Also the ELF header encodes a distinction between default and non-default. This allows the loader to enforce compatibility between multiple loaded code objects with different settings.
Yes, and the original plan was to keep the target features the same because the backend shouldn't care. The problem is that we need to enforce compatible features for every function, and in that sense "Default" and "On" have distinct meaning. The difference also matters when generating EFLAGs. We are adding a pass to check compatibility between target features, and "Default" is part of that equation.

For SramEcc "On", "Off", and "Default" will eventually generate different code.

It also makes sense to be consistent across the stack with the concepts in the new target ID.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:141
+  /// are Off, On, Default, and NotSupported.
+  virtual XnackSetting getXnackSetting() const;
+
----------------
arsenm wrote:
> No reason to use virtual for this
I wasn't sure if this might ever be used with AMDGPUSubtarget or only GCNSubtarget.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882