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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h:12
 
+#include "../Utils/AMDGPUPALMetadata.h"
 #include "AMDKernelCodeT.h"
----------------
I think `#include "Utils/AMDGPUPALMetadata.h"` should be enough?


================
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;
----------------
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`?


================
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);
----------------
Am I missing the use of this? Or is this here for when objdump can disassemble the note again?


================
Comment at: lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp:189-195
+    union {
+      char AsBytes[8];
+      uint32_t AsInts[2];
+    } U;
+    U.AsInts[0] = I.first;
+    U.AsInts[1] = I.second;
+    Blob.insert(Blob.size(), &U.AsBytes[0], 8);
----------------
I think this kind of pun is UB in C++, and this doesn't handle big-endian hosts. A `memcpy` would work, but would still require handling endianness. I think just masking out each individual byte handles everything.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57027