[PATCH] D34555: [NVPTX] Add lowering of i128 params.

[Artem Belevich via Phabricator via llvm-commits]

tra requested changes to this revision.
tra added a comment.
This revision now requires changes to proceed.

In general the patch looks OK.
One thing that you need to make sure is that alignment of i128 on nvptx side always matches that on the host side. AFAICT, i128 alignment on x64 is 16 (see https://reviews.llvm.org/D28990) , but this patch shows that it's 8 on nvptx side. That would result in different in-memory representation of aggregates between host and device.



================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.cpp:403
   if (isABI) {
-    if (Ty->isFloatingPointTy() || Ty->isIntegerTy()) {
+    if ((Ty->isFloatingPointTy() || Ty->isIntegerTy()) && !Ty->isIntegerTy(128)) {
       unsigned size = 0;
----------------
Nit. I'd group together `(Ty->isIntegerTy() && !Ty->isIntegerTy(128))`


================
Comment at: lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1428-1434
+  // Special case for i128
+  if (ETy->isIntegerTy(128)) {
+    O << " .b8 ";
+    getSymbol(GVar)->print(O, MAI);
+    O << "[16]";
+    return;
+  }
----------------
Please add a test case for this.



================
Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1279
     O << "(";
-    if (retTy->isFloatingPointTy() || retTy->isIntegerTy()) {
+    if ((retTy->isFloatingPointTy() || retTy->isIntegerTy()) && !retTy->isIntegerTy(128)) {
       unsigned size = 0;
----------------
Same nit as above.


https://reviews.llvm.org/D34555