[PATCH] D138531: [PATCH] [NVPTX] Backend support for variadic functions

[Pavel Kopyl via Phabricator via llvm-commits]

pavelkopyl added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1677
+    O << "\t.param .align " << STI.getMaxRequiredAlignment();
+    O << " .b8 %VAParam[]";
+  }
----------------
tra wrote:
> We should probably follow the naming convention we use for other arguments `<function>_paramN` or in this case, `<function>_vararg`.
OK, done.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1435
+
+  O.flush();
+  return Prototype;
----------------
tra wrote:
> Is this needed? AFAICT `raw_string_ostream` is unbuffered and everything just gets appended to the string with nothing to flush.
Yes, it's unbuffered. Fixed.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1594-1595
+                               Ty->isIntegerTy(128)));
+    } else if (IsByVal || (Ty->isAggregateType() || Ty->isVectorTy() ||
+                           Ty->isIntegerTy(128))) {
       // declare .param .align <align> .b8 .param<n>[<size>];
----------------
tra wrote:
> nor here.
Agree. Both statements are fixed. 


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1717-1718
+        if (!IsByVal && IsVAArg) {
+          assert(NumElts == 1 &&
+                 "Vectorization is expected to be disabled for variadics.");
+          VAOffset += DL.getTypeAllocSize(
----------------
tra wrote:
> In practice we may want/need to deal with f16x2 and bf16x2 as variadic arguments. While nominally they are vectors in IR, they are passed as scalars and thus we should be able to pass them as variadic arguments. It's OK to deal with this later, in which case, this should have a TODO comment.
Yes, probably in a perspective we will also support vectors in variadic arguments. I've added TODO about this.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:2337
 
+// This function is almostly a copy of SelectionDAG::expandVAArg().
+// The only diff is that this one produces loads from local address space.
----------------
tra wrote:
> `almostly`. It's a good one. :-)
> 
Fixed)


================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:200-203
+def VAParam32 :
+  NVPTXInst<(outs Int32Regs:$dst), (ins), "mov.u32 \t$dst, %VAParam;", []>;
+def VAParam64 :
+  NVPTXInst<(outs Int64Regs:$dst), (ins), "mov.u64 \t$dst, %VAParam;", []>;
----------------
tra wrote:
> Can we use existing `MOV_ADDR`/`MOV_ADDR64`  instead? It would also avoid hardcoding the symbol name.
> 
> That said, the fixed name has the benefit of being simpler, with the downside that the name we generate must be in sync with the name used by the instruction.
> 
> Another minor downside of hardcoded name is that it would be harder to search for in the generated PTX -- as it would be the same in all the functions. Having vararg argument name prefixed with function name as we do for other arguments would work better, IMO.
> 
Thank you for advice.
Done. The only thing is that technically I create (Wrapper texternalsym) DAG to select IMOV64ri or IMOV32ri instructions. This is how fixed-sized .param arrays are lowered.
To be selected, MOV_ADDR requires a bit different DAG - (Wrapper tglobaladdr). 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138531