[PATCH] D125285: [BuildLibCalls] infer inreg param attrs from NumRegisterParameters

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D125285#3504537 <https://reviews.llvm.org/D125285#3504537>, @nickdesaulniers wrote:

> In D125285#3504529 <https://reviews.llvm.org/D125285#3504529>, @efriedma wrote:
>
>> I can't find any libcalls with i64 arguments we emit in IR (on 32-bit targets).
>
> Should I drop the `getTypeAllocSize` related check for now, if we can't provide test coverage? (I guess I could write a unittest, but plz no).

I guess you could replace it with an assertion?  I think I'm fine leaving it without an explicit testcase, though.



================
Comment at: llvm/test/Transforms/InstCombine/simplify-libcalls-inreg.ll:47
+
+; CHECK:     declare noundef i32 @sprintf(ptr inreg noalias nocapture noundef writeonly, ptr inreg nocapture noundef readonly, ...)
+; CHECK-NOT: declare noundef i32 @sprintf(ptr noalias nocapture noundef writeonly, ptr nocapture noundef readonly, ...)
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > Is this inreg marking correct for sprintf?
> > > 
> > > Looking at clang output, we actually add inreg markings, but the backend ignores them.  I guess if we do the same thing, that's fine?
> > > Is this inreg marking correct for sprintf?
> > 
> > sprintf takes two ptr params, then the rest are variadic; yes?
> > 
> > > I guess if we do the same thing, that's fine?
> > 
> > I think so:
> > 
> > ```
> > // /tmp/sprintf.c
> > #include <stdio.h>
> > 
> > char a [20];
> > char b [20];
> > 
> > int x(void) {
> >   return sprintf(a, b, "hello");
> > }
> > ```
> > ```
> > $ gcc -m32 -mregparm=3 -S -o - -O2 /tmp/sprintf.c -fno-pic -fno-asynchronous-unwind-tables
> > ...
> > x:
> >         subl    $16, %esp
> >         pushl   $.LC0
> >         pushl   $b
> >         pushl   $a
> >         call    sprintf
> >         addl    $28, %esp
> >         ret
> > $ clang -m32 -mregparm=3 -S -o - -O2 /tmp/sprintf.c -fno-pic -fno-asynchronous-unwind-tables
> > ...
> > x:                                      # @x
> > # %bb.0:                                # %entry
> >         subl    $16, %esp
> >         pushl   $.L.str
> >         pushl   $b
> >         pushl   $a
> >         calll   sprintf
> >         addl    $28, %esp
> >         retl
> > ```
> Or would you prefer then that I set none of the params as inreg for variadic? That way we don't rely on the backend DTRT?
I think it makes sense not to set inreg if we don't expect it to actually do anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125285