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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5011
+      uint32_t Stepping;
+      char VendorAndArchitectureName[1];
+    };
----------------
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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5053
+    raw_string_ostream StrOS(MetadataString);
+    for (size_t i = 0; i < Desc.size() / sizeof(PALMetadata); ++i) {
+      StrOS << "[" << Isa[i].Key << ": " << Isa[i].Value << "]";
----------------
`i` -> `I`. Also LLVM style is to precalculate the end condition where possible. See inline edit.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5056
+    }
+    return {"AMD PAL Metadata", StrOS.str()};
+  }
----------------
Could you just return `MetadataString` directly? Similar comment in other cases.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6116
+      default:
+        // ELFOSABI_AMDGPU_PAL, ELFOSABI_AMDGPU_MESA3D support *_V3 flags.
+        LLVM_FALLTHROUGH;
----------------
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?


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

https://reviews.llvm.org/D95638