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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D95638#2531247 <https://reviews.llvm.org/D95638#2531247>, @kzhuravl wrote:

> 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.

Usually we have testing for llvm-readobj behaviour in test/tools/llvm-readobj, so that it can be kept independent of other testing, and so that changes in other areas don't impact test coverage for llvm-readobj. It also makes it easier to keep the tests focused on individual features. For example, what testing is there for EM_AMDGPU objects with an unrecognised ABI Version, or other aspects of that switch?



================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd.s:29
+// GNU-NEXT:     AMD PAL Metadata:
+// GNU-NEXT: {{^        $}}
 // GNU-EMPTY:
----------------
What are you trying to achieve with these checks? That there is explicit whitespace on this line and nothing else? That seems less than ideal to me. Why would you want that?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6209
+      default:
+        break;
+      case 0:
----------------
You need to print an empty Flags field here, for consistency with other output.


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

https://reviews.llvm.org/D95638