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

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl marked 10 inline comments as done.
kzhuravl added a comment.

In D95638#2530115 <https://reviews.llvm.org/D95638#2530115>, @jhenderson wrote:

> I've only looked at the llvm-readobj stuff: there are a very large number of changes to that tool in this change, but no direct testing (i.e. tests under llvm\test\llvm-readobj) that has been changed. I'm guessing it's not all covered by existing direct testing of the tool?

It is not covered by the direct testing of the tool (no tests in llvm\test\llvm-readobj), but has a very good coverage in llvm/test/CodeGen/AMDGPU.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:129
+  // emitFunctionBodyStart?
+  if (getTargetStreamer() && !getTargetStreamer()->getTargetID())
+    initializeTargetID(M);
----------------
arsenm wrote:
> Why check getTargetStreamer()? Why would htis ever be null? The uses below don't check it
Check is here because the following test is passing "-emit-codegen-only" option to cc1, which executes EmitCodeGenOnlyAction, which does not have MC including target streamer.

```
Clang :: Misc/backend-resource-limit-diagnostics.cl
```

The uses below never check it because backend-resource-limit-diagnostics.cl does not specify the os in the triple.

Do you have better suggestions on how to work around it?


================
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") {
----------------
arsenm wrote:
> StringSwitch? Also check the prefix and drop gfx?
Do you suggest converting the whole if-else block into StringSwitch? If yes, how would errors be handled?

In addition there is an else statement saying we do not support the processor in v2:

```
...
      } else if (Processor == "gfx906") {
        if (isXnackOnOrAny())
          Processor = "gfx907";
      } else {
        report_fatal_error(
            "AMD GPU code object V2 does not support processor " + Processor);
      }
```


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5011
+      uint32_t Stepping;
+      char VendorAndArchitectureName[1];
+    };
----------------
jhenderson wrote:
> Whilst I follow what's going on here after reading more carefully, the single byte array is confusing to me. Is there a particular reason for doing it this way, rather than just omitting it and using `Desc.data() + sizeof(IsaVersion)`? The latter seems more obvious to me.
I guess this was done to convey more readability. But since it failed, I have changed to the way you suggested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6116
+      default:
+        // ELFOSABI_AMDGPU_PAL, ELFOSABI_AMDGPU_MESA3D support *_V3 flags.
+        LLVM_FALLTHROUGH;
----------------
jhenderson wrote:
> It seems to me like there's potential for other versions either now or in the future that don't support the V3 flags?  Is there a risk this default case will be unintentionally hit in those cases?
I think being more explicit is good. Thanks!


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

https://reviews.llvm.org/D95638