[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:65
+  enum XnackSetting {
+    XnackDefault = 1,
+    XnackOn = 2,
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882