[PATCH] D147143: Add backend support for new PAL ELF Metadata 3.0

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus added a comment.

I don't know metadata handling enough to do a proper review, but I found some inline nits.



================
Comment at: llvm/lib/Target/AMDGPU/SIProgramInfo.h:39
 
-    // Fields set in PGM_RSRC2 pm4 packet.
+    // State used to calculate Fields set in PGM_RSRC2 pm4 packet.
     uint32_t LDSBlocks = 0;
----------------
`Fields` -> `fields`?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp:916
+
+unsigned AMDGPUPALMetadata::getPALVersion(unsigned idx) {
+  if (!VersionChecked) {
----------------
Maybe add `assert(idx < 2);`?
Or even use an enum, but that may be overkill :)


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h:147
+  // nothing if the field does not exist.
+  std::optional<std::reference_wrapper<msgpack::DocNode>>
+  refComputeRegister(StringRef field);
----------------
Why not return a `msgpack::DocNode*` which may be `nullptr`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147143