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

[David Stuttard via Phabricator via llvm-commits]

dstuttard added inline comments.


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


================
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);
----------------
jsilvanus wrote:
> Why not return a `msgpack::DocNode*` which may be `nullptr`?
You are right - I'd got too fixated on making the function work like all the others, but the simplest approach is to go with a pointer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147143