[BuildLibCalls] infer inreg param attrs from NumRegisterParameters

[nickdesaulniers]

Summary

We're having a hard time booting the ARCH=i386 Linux kernel with clang
after removing -ffreestanding because instcombine was dropping inreg
from callers during libcall simplification, but not the callees defined
in different translation units. This led the callers and callees to have
wildly different calling conventions, which (predictably) blew up at
runtime.

Infer the inreg param attrs on function declarations from the module
metadata "NumRegisterParameters." This allows us to boot the ARCH=i386
Linux kernel (w/ -ffreestanding removed).

Fixes: https://github.com/llvm/llvm-project/issues/53645

[efriedma]

Do you really want to mark all arguments? The equivalent code in X86TargetLowering::markLibCallAttributes only marks int/pointer arguments. (I guess in the kernel, all arguments to relevant library functions are integers or pointers, but in userspace, you're likely to run into other stuff like floats.)

[nickdesaulniers]

• rewrite logic to closer match X86TargetLowering::markLibCallAttributes

[nickdesaulniers]

• remove unused include, save Module*

[efriedma]

Do you need to exclude varargs functions?

Please add tests for:

ldexp
snprintf (or some other varargs function)

[nickdesaulniers]

• add tests for non-int params (ldexp) and varargs (sprintf)

[nickdesaulniers]

Do you need to exclude varargs functions?

I didn't need to change anything in my logic; seems to work.

Please add tests for:

Done, thanks. Got anything that might emit a libcall with an i64? I was looking at ffsl but AFAICT, we don't emit that.

[efriedma]

llvm/test/Transforms/InstCombine/simplify-libcalls-inreg.ll

48

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?

[efriedma]

I can't find any libcalls with i64 arguments we emit in IR (on 32-bit targets).

[nickdesaulniers]

llvm/test/Transforms/InstCombine/simplify-libcalls-inreg.ll

48

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

[nickdesaulniers]

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

llvm/test/Transforms/InstCombine/simplify-libcalls-inreg.ll

48

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?

[efriedma]

In D125285#3504537, @nickdesaulniers wrote:

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

llvm/test/Transforms/InstCombine/simplify-libcalls-inreg.ll

48

I think it makes sense not to set inreg if we don't expect it to actually do anything.

[nickdesaulniers]

• don't re-attribute varargs, add assertion for params larger than word size

[nickdesaulniers]

In D125285#3504802, @efriedma wrote:

In D125285#3504537, @nickdesaulniers wrote:

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

I've left the logic, but added the assertion. Please let me know if you'd prefer that I simplified the logic based on the assertion.

[efriedma]

LGTM with one minor comment

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

1233

This could probably use a better name to indicate what it's doing. Maybe markRegisterParameterAttributes()

[nickdesaulniers]

• rename fn

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/BuildLibCalls.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/IR/Argument.h"
+#include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
+#include "llvm/Support/TypeSize.h"
+
+#include <algorithm>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "build-libcalls"
 
 //- Infer Attributes ---------------------------------------------------------//
 
 STATISTIC(NumReadNone, "Number of functions inferred as readnone");

 
 static void setArgExtAttr(Function &F, unsigned ArgNo,
                           const TargetLibraryInfo &TLI, bool Signed = true) {
   Attribute::AttrKind ExtAttr = TLI.getExtAttrForI32Param(Signed);
   if (ExtAttr != Attribute::None && !F.hasParamAttribute(ArgNo, ExtAttr))
     F.addParamAttr(ArgNo, ExtAttr);
 }
 
+// Modeled after X86TargetLowering::markLibCallAttributes.
+static void markLibCallAttributes(Function *F) {
+  if (!F->arg_size())
+    return;
+
+  const CallingConv::ID CC = F->getCallingConv();
+  if (CC != CallingConv::C && CC != CallingConv::X86_StdCall)
+    return;
+
+  unsigned N = F->getParent()->getNumberRegisterParameters();
+  if (!N)
+    return;
+
+  const DataLayout &DL = F->getParent()->getDataLayout();
+
+  for (Argument &A : F->args()) {
+    Type *T = A.getType();
+    if (!T->isIntOrPtrTy())
+      continue;
+
+    const TypeSize &TS = DL.getTypeAllocSize(T);
+    if (TS > 8)
+      continue;
+
+    const unsigned NumRegs = TS > 4 ? 2 : 1;
+    if (N < NumRegs)
+      return;
+
+    N -= NumRegs;
+    F->addParamAttr(A.getArgNo(), Attribute::InReg);
+  }
+}
+
 FunctionCallee llvm::getOrInsertLibFunc(Module *M, const TargetLibraryInfo &TLI,
                                         LibFunc TheLibFunc, FunctionType *T,
                                         AttributeList AttributeList) {
   assert(TLI.has(TheLibFunc) &&
          "Creating call to non-existing library function.");
   StringRef Name = TLI.getName(TheLibFunc);
   FunctionCallee C = M->getOrInsertFunction(Name, T, AttributeList);
 

 #ifndef NDEBUG
     for (unsigned i = 0; i < T->getNumParams(); i++)
       assert(!isa<IntegerType>(T->getParamType(i)) &&
              "Unhandled integer argument.");
 #endif
     break;
   }
 
+  markLibCallAttributes(F);
+
   return C;
 }
 
 FunctionCallee llvm::getOrInsertLibFunc(Module *M, const TargetLibraryInfo &TLI,
                                         LibFunc TheLibFunc, FunctionType *T) {
   return getOrInsertLibFunc(M, TLI, TheLibFunc, T, AttributeList());
 }
 

llvm/test/Transforms/InstCombine/simplify-libcalls-inreg.ll

+; RUN: opt -passes=instcombine -S %s | FileCheck %s
+
+; The intent of this test is to check that the declarations produces for
+; libcalls retains the inreg parameter attribute.
+
+target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
+target triple = "i386-unknown-linux-gnu"
+
+declare ptr @foo()
+declare i32 @memcmp(ptr inreg nocapture noundef, ptr inreg nocapture noundef, i32 inreg noundef)
+declare i32 @printf(i8*, ...)
+
+; CHECK:     declare i32 @bcmp(ptr inreg nocapture, ptr inreg nocapture, i32 inreg)
+; CHECK-NOT: declare i32 @bcmp(ptr nocapture, ptr nocapture, i32)
+
+define i32 @baz(ptr inreg noundef %s2, i32 inreg noundef %n){
+  %call = call ptr @foo()
+  %call1 = call i32 @memcmp(ptr inreg noundef %call, ptr inreg noundef %s2, i32 inreg noundef %n)
+  %cmp = icmp eq i32 %call1, 0
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+}
+
+; CHECK:     declare noundef i32 @putchar(i32 inreg noundef)
+; CHECK-NOT: declare noundef i32 @putchar(i32 noundef)
+
+@h = constant [2 x i8] c"h\00"
+
+define void @test_simplify2() {
+  %fmt = getelementptr [2 x i8], [2 x i8]* @h, i32 0, i32 0
+  call i32 (i8*, ...) @printf(i8* %fmt)
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"NumRegisterParameters", i32 3}