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

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


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


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


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1592
+      }
+      NeedAlign = (IsByVal || (Ty->isAggregateType() || Ty->isVectorTy() ||
+                               Ty->isIntegerTy(128)));
----------------
Nit: None of `()` are needed here.


================
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>];
----------------
nor here.


================
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(
----------------
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.


================
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.
----------------
`almostly`. It's a good one. :-)



================
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;", []>;
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138531