[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.

General nit: patch should be submitted on phabricator using large context. Please see: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

All i*-param.ll tests should probably be combined into one test file. They do pretty much the same thing.

Also, there should probably be test cases for integer sizes that are not multiples of 8. E.g `i49`.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1500-1503
+        if (sz <= 32)
           sz = 32;
+        else if (sz <= 64)
+          sz = 64;
----------------
Size roundup calculation appears to be repeated in many places and could be extracted into a helper function.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:216
+  if (VT.isScalarInteger()) {
+    if (1 < VT.getFixedSizeInBits() && VT.getFixedSizeInBits() < 8) {
+      *PromotedVT = MVT::i8;
----------------
This looks a bit odd -- checking for the same boundary in two places, implicitly skipping power-of-2 sized integers...

I'd restructure it a bit to look like this:
```
if (VT.isScalarInteger()) {
   auto n = VT.getFixedSizeInBits();
   if (isPowerOf2_32(n))
     return false;
   switch(PowerOf2Ceil(n)){
      default: return false; // Covers i1 and integers larger than i64
      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 true
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129291