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

[Scott Linder via Phabricator via llvm-commits]

scott.linder accepted this revision.
scott.linder added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! I think you can also drop "provides a method for use by LLPC to write metadata into LLVM IR" from the commit message.



================
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;
----------------
tpr wrote:
> 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.
That make sense, but what about cases where `Tuple->getNumOperands()` is odd? It seems like values are just silently ignored? In the end the values are probably not being edited, and you will be deleting this code, so I'm OK with dropping the odd value.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57027