[PATCH] D95638: AMDGPU: Add target id and code object v4 support

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPU.td:873
    FeatureLDSBankCount32,
-   FeatureXNACK,
    FeatureImageGather4D16Bug]>;
----------------
This looks like a separate change


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:129
+  // emitFunctionBodyStart?
+  if (getTargetStreamer() && !getTargetStreamer()->getTargetID())
+    initializeTargetID(M);
----------------
Why check getTargetStreamer()? Why would htis ever be null? The uses below don't check it


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:207-208
+      FunctionTargetID.getXnackSetting() != getTargetStreamer()->getTargetID()->getXnackSetting())
+    report_fatal_error("xnack setting of '" + Twine(MF->getName()) +
+                       "' function does not match module xnack setting");
+  // Make sure function's sramecc settings are compatible with module's
----------------
Probably should use proper context errors here


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:623
+    const IsaInfo::AMDGPUTargetID &TID = STM.getTargetID();
+    if (getTargetStreamer()->getTargetID()->isXnackSupported())
+      if (getTargetStreamer()->getTargetID()->getXnackSetting() == IsaInfo::TargetIDSetting::Any)
----------------
Should use a temp var for getTargetStreaemr()->getTargetId() instead of repeating it so many times


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5121
 
-  if (Subtarget->getTrapHandlerAbi() != GCNSubtarget::TrapHandlerAbiHsa ||
-      !Subtarget->isTrapHandlerEnabled())
-    return DAG.getNode(AMDGPUISD::ENDPGM, SL, MVT::Other, Chain);
+SDValue SITargetLowering::lowerTRAP_AMDHSA_QUEUE_PTR(
+    SDValue Op, SelectionDAG &DAG) const {
----------------
Weird naming scheme. lowerTrapHSAQueuePtr?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:381-390
+      if (Processor == "gfx600") {
+      } else if (Processor == "gfx601") {
+      } else if (Processor == "gfx602") {
+      } else if (Processor == "gfx700") {
+      } else if (Processor == "gfx701") {
+      } else if (Processor == "gfx702") {
+      } else if (Processor == "gfx703") {
----------------
StringSwitch? Also check the prefix and drop gfx?


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

https://reviews.llvm.org/D95638