Page MenuHomePhabricator

[BuildLibCalls] infer inreg param attrs from NumRegisterParameters
ClosedPublic

Authored by nickdesaulniers on May 9 2022, 6:47 PM.

Details

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

Diff Detail

Event Timeline

nickdesaulniers created this revision.May 9 2022, 6:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 6:47 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
nickdesaulniers requested review of this revision.May 9 2022, 6:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 6:47 PM

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

  • rewrite logic to closer match X86TargetLowering::markLibCallAttributes
  • remove unused include, save Module*

Do you need to exclude varargs functions?

Please add tests for:

ldexp
snprintf (or some other varargs function)

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

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 added inline comments.May 10 2022, 1:33 PM
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?

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

nickdesaulniers marked an inline comment as done.May 10 2022, 1:41 PM
nickdesaulniers added inline comments.
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 marked an inline comment as done.May 10 2022, 1:46 PM

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?

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 marked an inline comment as done.
  • don't re-attribute varargs, add assertion for params larger than word size

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 accepted this revision.May 10 2022, 4:12 PM

LGTM with one minor comment

llvm/lib/Transforms/Utils/BuildLibCalls.cpp
1231

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

This revision is now accepted and ready to land.May 10 2022, 4:12 PM
nickdesaulniers marked an inline comment as done.May 10 2022, 4:20 PM
efriedma accepted this revision.May 10 2022, 4:24 PM
This revision was landed with ongoing or failed builds.May 10 2022, 4:36 PM
This revision was automatically updated to reflect the committed changes.