[PATCH] D47736: AMDHSA Code Object v3 assembler syntax update

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
Comment at: include/llvm/Support/AMDHSAKernelDescriptor.h:153-154
 
+/// Overwrite \p KD with default values.
+void setDefaultKernelDescriptor(kernel_descriptor_t &KD);
+
----------------
Can this be moved to lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h/cpp?


================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:2631
+
+  // TODO(scott.linder): should this be in streamIsaVersion?
+  if (hasXNACK())
----------------
Yes.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:137
+void AMDGPUTargetAsmStreamer::EmitDirectiveAMDGCNTarget(StringRef Target) {
+  // TODO: quote/escape Target?
+  OS << "\t.amdgcn_target \"" << Target << "\"\n";
----------------
My guess would be yes for this one, since we do it in other similar places.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:221
+
+  // TODO: quote/escape KernelName?
+  OS << "\t.amdhsa_kernel " << KernelName << '\n';
----------------
probably no, as our current directive *amdgpu_hsa_kernel* does not do that.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h:71
+      StringRef KernelName, const amdhsa::kernel_descriptor_t &KernelDescriptor,
+      const MCSubtargetInfo &STI) = 0;
 };
----------------
Can STI be made the first argument?


https://reviews.llvm.org/D47736