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

[Kjetil Kjeka via Phabricator via llvm-commits]

kjetilkjeka marked an inline comment as done.
kjetilkjeka added a comment.

Thanks for being so responsive and helpful! It helps a lot being my first time attempting to contribute to LLVM.

I added two small questions to your comments as inline comments.

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

Should I then also remove the attempts of testing the truncating/extending to make all tests the same?



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:217
+    auto n = VT.getFixedSizeInBits();
+    if (n >= 8 && isPowerOf2_32(n))
+      return false;
----------------
tra wrote:
> 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;
> ```
I will fix this together with the change from the other comment when I have gotten your opinion on where to put the helper from the other comment but first just a question about the `case default`.

Isn't it better to keep the `case default: return false;` to allow for attempting to promote the EVT and doing somewhere else if there is not a proper way to do it? I'm thinking more of the future scenario where things like `i99` and `i1000` is supported and you might first try to promote the integer type and if it fail then instead try to split it up into parts. I guess it doesn't make a big difference for this patch because things like `i99` or `i1000` will fail to compile both before and after the patch anyway. The only practical difference is the type of error you get if you try to feed these kind of integers to `llc`.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1327-1330
       if (size < 32)
         size = 32;
+      else if (size > 32 && size < 64)
+        size = 64;
----------------
tra wrote:
> Use promoteScalarArgumentSize() here, too?
We would then need to put `promoteScalarArgumentSize` in a place that would be visible from both files (`NVPTXSelLowering.cpp` and `NVPTXAsmPrinter.cpp`). Would it make sense to use `NVPTXUtilities.cpp` for that?


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

https://reviews.llvm.org/D129291