[PATCH] D57027: [AMDGPU] Factored PAL metadata handling out into its own class

[Tim Renouf via Phabricator via llvm-commits]

tpr marked 4 inline comments as done.
tpr added inline comments.


================
Comment at: lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp:41-45
+  for (unsigned I = 0, E = Tuple->getNumOperands() & -2; I != E; I += 2) {
+    auto Key = mdconst::dyn_extract<ConstantInt>(Tuple->getOperand(I));
+    auto Val = mdconst::dyn_extract<ConstantInt>(Tuple->getOperand(I + 1));
+    if (!Key || !Val)
+      continue;
----------------
scott.linder wrote:
> Even if you keep going with whatever you can get out of the IR, if it is malformed I would emit some sort of diagnostic. Maybe return a `bool` like in `setFromBlob` and emit a diagnostic in `AMDGPUAsmPrinter::EmitStartOfAsmFile`?
Val==0 is certainly not malformed, it's just that we don't need a map entry for a 0 value register. Key==0 is arguably malformed, but I think LLPC actually does it, so I would rather not emit a diagnostic for it.


================
Comment at: lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp:53
+// Metadata.
+bool AMDGPUPALMetadata::setFromBlob(unsigned Type, StringRef Blob) {
+  assert(Type == ELF::NT_AMD_AMDGPU_PAL_METADATA);
----------------
scott.linder wrote:
> Am I missing the use of this? Or is this here for when objdump can disassemble the note again?
Yes, we want objdump to use it again one day via a new target abstraction, and we have an out-of-tree utility that uses it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57027