[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:65
+  enum XnackSetting {
+    XnackDefault = 1,
+    XnackOn = 2,
----------------
kerbowa wrote:
> 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.
This is the first I've heard of a pass to check compatibility. I hope this is at most a verifier tool


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882