[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.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");
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:910-924
+  if (XnackOnSupportRequested && !SupportsXnack)
+    report_fatal_error(
+        "Xnack On was requested for a processor that does not support it!");
+
+  if (SramEccOnSupportRequested && !SupportsSramEcc)
+    report_fatal_error(
+        "SramEcc On was requested for a processor that does not support it!");
----------------
I don't think these should be fatal errors. SubtargetFeature is really missing ImpliesNot


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:943
+    SramEccSetting SramEcc = getSramEccSetting();
+
+    dbgs() << "XNACK setting for subtarget: ";
----------------
I'm not sure we want debug printing in the subtarget constructor


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:944-958
+    dbgs() << "XNACK setting for subtarget: ";
+    switch (Xnack) {
+    case (AMDGPUSubtarget::XnackOn):
+      dbgs() << "On";
+      break;
+    case (AMDGPUSubtarget::XnackOff):
+      dbgs() << "Off";
----------------
StringSwitch


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:962-975
+    switch (SramEcc) {
+    case (AMDGPUSubtarget::SramEccOn):
+      dbgs() << "On";
+      break;
+    case (AMDGPUSubtarget::SramEccOff):
+      dbgs() << "Off";
+      break;
----------------
dbgs() << StringSwitch


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:976
+    };
+    dbgs() << "\n";
+  });
----------------
Single quotes


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:65
+  enum XnackSetting {
+    XnackDefault = 1,
+    XnackOn = 2,
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:141
+  /// are Off, On, Default, and NotSupported.
+  virtual XnackSetting getXnackSetting() const;
+
----------------
No reason to use virtual for this


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:500
+bool isXnackSupported(const MCSubtargetInfo *STI) {
+  AMDGPU::GPUKind AK = parseArchAMDGCN(STI->getCPU());
+  if (AK == AMDGPU::GPUKind::GK_NONE)
----------------
Parsing shouldn't be necessary


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


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:529
+bool isSramEccSupported(const MCSubtargetInfo *STI) {
+  AMDGPU::GPUKind AK = parseArchAMDGCN(STI->getCPU());
+  if (AK == AMDGPU::GPUKind::GK_NONE)
----------------
Parsing shouldn't be necessary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85882