[PATCH] D37753: [AMDGPU] implemented pal metadata

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:216
+void AMDGPUAsmPrinter::readPalMetadata(Module &M) {
+  if (auto NamedMD = M.getNamedMetadata("_amdgpu_pal_metadata"))
+    if (NamedMD->getNumOperands())
----------------
arsenm wrote:
> These can all be early returns to reduce indentation
The usual metadata naming convention uses . separators without leading _ (I also don't see any tests using this)


================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:216-218
+  if (auto NamedMD = M.getNamedMetadata("_amdgpu_pal_metadata"))
+    if (NamedMD->getNumOperands())
+      if (auto Tuple = dyn_cast<MDTuple>(NamedMD->getOperand(0)))
----------------
These can all be early returns to reduce indentation


================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1022
+      PalMetadata[Rsrc2Reg] |= S_00B84C_SCRATCH_EN(1);
+    if (STM.isVGPRSpillingEnabled(*MF.getFunction())) {
+      // ScratchSize is in bytes -- metadata wants it in dwords, rounded up to
----------------
You shouldn't be using this here (or anywhere really). We definitely know the stack size at this point.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:119
+    OS << (I == Data.begin() ? " 0x" : ",0x") << Twine::utohexstr(*I);
+  OS << "\n";
+  return true;
----------------
Single quotes


https://reviews.llvm.org/D37753