[PATCH] D158579: [AMDGPU] Add DAG ISel support for preloaded kernel arguments

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUArgumentUsageInfo.cpp:63
 
+// TODO: Print preload kernargs?
 void AMDGPUArgumentUsageInfo::print(raw_ostream &OS, const Module *M) const {
----------------
Probably should but I don't even know if this is part of debug printing anywhere, I don't know the last time I saw this


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:463
 
+  if (STM.hasGFX90AInsts())
+    KernelDescriptor.kernarg_preload =
----------------
Can you move this to hasKernargPreload helper or something? Probably should make it a full subtarget feature on its own


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp:261-263
+            LLVMContext::MD_dereferenceable,
+            MDNode::get(Ctx, MDB.createConstant(ConstantInt::get(
+                                 Builder.getInt64Ty(), DerefBytes))));
----------------
Can you separate all the formatting changes out?


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5003
+    } else if (ID == ".amdhsa_user_sgpr_kernarg_preload_length") {
+      if (Val > 16)
+        return OutOfRangeError(ValRange);
----------------
arsenm wrote:
> Is the interaction with   .amdhsa_user_sgpr_count checked?
Check the subtarget for the limit


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5003-5004
+    } else if (ID == ".amdhsa_user_sgpr_kernarg_preload_length") {
+      if (Val > 16)
+        return OutOfRangeError(ValRange);
+      PARSE_BITS_ENTRY(KD.kernarg_preload, KERNARG_PRELOAD_SPEC_LENGTH, Val,
----------------
Is the interaction with   .amdhsa_user_sgpr_count checked?


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1949-1955
+    using namespace amdhsa;
+    TwoByteBuffer = DE.getU16(Cursor);
+    PRINT_DIRECTIVE(".amdhsa_user_sgpr_kernarg_preload_length",
+                    KERNARG_PRELOAD_SPEC_LENGTH);
+    PRINT_DIRECTIVE(".amdhsa_user_sgpr_kernarg_preload_offset",
+                    KERNARG_PRELOAD_SPEC_OFFSET);
+    return MCDisassembler::Success;
----------------
Can you split the assembler / MC bits into a separate patch?


================
Comment at: llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:904-906
+                MCSymbolRefExpr::create(KernelCodeSymbol,
+                                        MCSymbolRefExpr::VK_AMDGPU_REL64,
+                                        Context),
----------------
Needs some temporary variables


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:2235
+
+void SITargetLowering::allocatePreloadKernArgSGPRs(
+    CCState &CCInfo, SmallVectorImpl<CCValAssign> &ArgLocs,
----------------
I do not understand this metadata system, you know directly from the IR arguments the register layout 


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:2245
+
+  if (!dyn_cast<MDNode>(MD->operands().begin()->get()))
+    return;
----------------
unchecked dyn_caast


================
Comment at: llvm/test/MC/AMDGPU/hsa-v4.s:113
   .amdhsa_system_vgpr_workitem_id 1
   .amdhsa_next_free_vgpr 9
   .amdhsa_next_free_sgpr 27
----------------
Need some tests with the limit exceeded diagnostic and interactions with amdhsa_user_sgpr_count


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158579