-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[BPF] Allow libcalls behind a feature gate #168442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@yonghong-song gentle ping |
vadorovsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
Could you add the [BPF] tag and description to your commit message as well?
| if (!AllowBuiltinCalls) { | ||
| fail(CLI.DL, DAG, | ||
| Twine("A call to built-in function '" + StringRef(E->getSymbol()) + | ||
| "' is not supported.")); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: usually if statements with one-liners are written without braces in the BPF backend code:
| if (!AllowBuiltinCalls) { | |
| fail(CLI.DL, DAG, | |
| Twine("A call to built-in function '" + StringRef(E->getSymbol()) + | |
| "' is not supported.")); | |
| } | |
| if (!AllowBuiltinCalls) | |
| fail(CLI.DL, DAG, | |
| Twine("A call to built-in function '" + StringRef(E->getSymbol()) + | |
| "' is not supported.")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a simple change, I'll add it to the existing commit in this PR and force push it again with the correct title you asked for.
🐧 Linux x64 Test Results
|
4808ce5 to
a1e74bf
Compare
yonghong-song
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain how bpf program handles builtin function like __multi3() in rust environment. Currently the builtin function like __multi3() won't work for libbpf and kernel as there is no actual function for __multi3() in final bpf program.
| EmitInstrWithCustomInserterLDimm64(MachineInstr &MI, | ||
| MachineBasicBlock *BB) const; | ||
|
|
||
| /// Returns true if arguments should be sign-extended in lib calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us do
// Returns true if arguments should be sign-extended in lib calls.
similar to the comments in previous lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I miss anything or is your suggestion the same thing as what I wrote there already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I didn't see you were using two slashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the comment in the squashed commit.
| ; CHECK: r5 s>>= 63 | ||
| ; CHECK: r1 = r10 | ||
| ; CHECK: r1 += -16 | ||
| ; CHECK: call __multi3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain in the commit/description message how do you handle __multi3 function? This is clang built-in so you will implement this function in the bpf prog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we build the core library in Rust, which implements the primitive types and their math operations, we also build as a dependency Rust's compiler-builtin crate. This crate has the implementation of __multi3 (see https://github.com/rust-lang/compiler-builtins/blob/eba1a3f3d2824739f07fe434624b0d2fee917d89/compiler-builtins/src/int/mul.rs#L108-L110).
Once we build the target program and all the dependencies (core and compiler-builtins), we merge all LLVM modules and emit a final object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this message clear for the commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the commit message. Let me know if it is clear. I'm happy to re-write it.
llvm/test/CodeGen/BPF/struct_ret2.ll
Outdated
| ; RUN: not llc -mtriple=bpf < %s 2> %t1 | ||
| ; RUN: FileCheck %s < %t1 | ||
| ; CHECK: only small returns | ||
| ; CHECK: aggregate returns are not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For struct_ret2.ll, the following is the error message:
$ llc -march=bpf test/CodeGen/BPF/struct_ret2.ll
error: <unknown>:0:0: in function foo { i64, i32 } (i32, i32, i32): 0x3d86ac0: i64 = GlobalAddress<ptr @bar> 0 too many arguments
error: <unknown>:0:0: in function foo { i64, i32 } (i32, i32, i32): aggregate returns are not supported
I suggest to have the first error message for CHECK, i.e., 'too many arguments" which is caused by CanLowerReturn().
Please also update struct_ret1.ll like below:
$ llc -march=bpf test/CodeGen/BPF/struct_ret1.ll
error: <unknown>:0:0: in function bar { i64, i32 } (i32, i32, i32, i32, i32): stack arguments are not supported
error: <unknown>:0:0: in function bar { i64, i32 } (i32, i32, i32, i32, i32): aggregate returns are not supported
error: <unknown>:0:0: in function baz void (ptr): aggregate returns are not supported
Similarly, I would like the error message in CHECK should be the first error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed both tests in the squashed commit.
When we build the core library in Rust, which implements the primitive types an their math operations, we also build as a dependency Rust's compiler-builtin[0] crate. This crate has the implementation of __multi3 [1]. Once we build the target program and all the dependencies (core and compiler-builtins), we merge all LLVM modules and emit a final object. [0] https://github.com/rust-lang/compiler-builtins [1] https://github.com/rust-lang/compiler-builtins/blob/eba1a3f3d2824739f07fe434624b0d2fee917d89/compiler-builtins/src/int/mul.rs#L108-L110
a1e74bf to
c9e6e0c
Compare
yonghong-song
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
Problem
In Rust, checked math functions (like
checked_mul,overflowing_mul,saturating_mul) are part of the primitive implementation of integers (see u64 for instance). The Rust compiler builds the Rust compiler-builtins crate as a step in the compilation processes, since it contains the math builtins to be lowered in the target.For BPF, however, when using those functions in Rust we hit the following errors:
Those errors come from the following code:
Those functions invoke underneath the llvm instrinc
{ i64, i1 } @llvm.umul.with.overflow.i64(i64, i64)or its variants.It is very useful to use safe math operations when writing BPF code in Rust, and I would like to add support for those in the target.
Changes
allow-builtin-callsto enable code generation for builtin functions.CanLowerReturnto fix the erroronly small returns supported.