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

In D129291#3647790 <https://reviews.llvm.org/D129291#3647790>, @kjetilkjeka wrote:

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

If you want to. Or it could be cleaned up in a separate patch. Leaving existing tests as is is fine, too.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:217
+    auto n = VT.getFixedSizeInBits();
+    if (n >= 8 && isPowerOf2_32(n))
+      return false;
----------------
kjetilkjeka wrote:
> 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`.
> 
> 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?

If there's a valid use case for it, then sure. On the other hand, if we are only expected to handle specific set of types (i1-i64), then I'd prefer to know right away if we ever end up with something that we can't handle. It's better to fail right away rather than ignore it and then have to debug it the hard way when things go wrong somewhere downstream. If we do grow a legitimate need to pass through other types unpromoted it will be easy enough to change.
 


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1327-1330
       if (size < 32)
         size = 32;
+      else if (size > 32 && size < 64)
+        size = 64;
----------------
kjetilkjeka wrote:
> 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?
I think you could just put it as an inline function into `NVPTXUtilities.h`


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

https://reviews.llvm.org/D129291