[PATCH] D129291: [NVPTX] Promote i24, i40, i48 and i56 to next power-of-two register when passing

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

> I have tried to address all the requested changes and created the new diff with the large context.

Thank you. It makes patch reviewing much easier.

> I added a couple of non standard tests, but LLVM seem to handle the promote/truncate in many different ways for the different non standard integers so I'm not sure at what level of detail it makes sense to test for here. Let me know if you want more tests.

We want to make sure that param loads/stores are done using correct sizes. We do not really care how LLVM does extension/truncation of non-power-of-2 sized integers.
So, in practice we only need to check for relevant ld.param/st.param instructions.
We don't need to enumerate all possible sizes. Something like i3/i11/i23/i47 should be sufficient to exercise the code in this patch.
We do have `test/CodeGen/NVPTX/param-load-store.ll` so the new test cases should probably go there as well.

BTW, for IR test cases here it's convenient to get the function to call itself, so you do not need both caller and callee and can test all relevant functionality in one place.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:217
+    auto n = VT.getFixedSizeInBits();
+    if (n >= 8 && isPowerOf2_32(n))
+      return false;
----------------
I think we may simplify it a bit further. Get rid of this check, always set PromotedVT to the appropriate size and return true if the type changed.
```
switch(PowerOf2Ceil(n)){
      default: llvm_unreachable();
      case 1:      *PromotedVT = MVT::i1; break;
      case 2:
      case 4:
      case 8:       *PromotedVT = MVT::i8; break;
      case 16:     *PromotedVT = MVT::i16; break;
      case 32:     *PromotedVT = MVT::i32; break;
      case 64:     *PromotedVT = MVT::i64; break;
    }
    return EVT(*PromotedVT) != VT;
```


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1327-1330
       if (size < 32)
         size = 32;
+      else if (size > 32 && size < 64)
+        size = 64;
----------------
Use promoteScalarArgumentSize() here, too?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1381-1384
         if (sz < 32)
           sz = 32;
+        else if (sz > 32 && sz < 64)
+          sz = 64;
----------------
ditto.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1555-1562
       if ((VT.isInteger() || VT.isFloatingPoint()) && TypeSize < 4) {
         // PTX ABI requires integral types to be at least 32 bits in
         // size. FP16 is loaded/stored using i16, so it's handled
         // here as well.
         TypeSize = 4;
+      } else if (VT.isInteger() && TypeSize > 4 && TypeSize < 8) {
+        TypeSize = 8;
----------------
This could also use promoteScalarArgumentSize().
```
if ((VT.isInteger() || VT.isFloatingPoint())
  TypeSize = promoteScalarArgumentSize(TypeSize*8)/8.
```


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1690-1693
       if (resultsz < 32)
         resultsz = 32;
+      else if (resultsz > 32 && resultsz < 64)
+        resultsz = 64;
----------------
ditto.


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

https://reviews.llvm.org/D129291