[LLVM-C] Add LLVMCreateTargetMachineWithABI #68406
Conversation
sebastianpoeplau
force-pushed
the
add-llvm-create-target-machine-with-abi
branch
2 times, most recently
from
c898472 to
c574067
Compare
|
with so many params, I think for future-proofing the API we should go the route of having an |
|
I like that 👍 Will work on it. |
sebastianpoeplau
force-pushed
the
add-llvm-create-target-machine-with-abi
branch
from
c574067 to
0ce4173
Compare
|
Done, modeled after the pass builder interface: Users create and manipulate an opaque option structure, then pass it to the new function |
llvm/lib/Target/TargetMachineC.cpp
Outdated
| case LLVMCodeGenLevelAggressive: | ||
| OL = CodeGenOptLevel::Aggressive; | ||
| break; | ||
| default: |
There was a problem hiding this comment.
we shouldn't have a default, we should report_fatal_error() if the user passes an invalid value in
There was a problem hiding this comment.
Oh wow, GitHub says you reviewed my change on Oct 18, but it didn't show me the review until now 😳 Anyway, I copied the default from the previous implementation, but I agree that raising an error makes more sense; will change it.
There was a problem hiding this comment.
The default was actually a way to handle LLVMCodeGenLevelDefault; I've handled that value explicitly and left the switch statement without a default, so that the compiler now warns if the enum is extended. Same for the translation of the LLVMRelocMode.
sebastianpoeplau
force-pushed
the
add-llvm-create-target-machine-with-abi
branch
3 times, most recently
from
2e969b2 to
52aacfe
Compare
|
looks good, but we really should have tests like |
| // Get the default target to keep the test as generic as possible. This may | ||
| // not be a target for which we can generate code; in that case we give up. | ||
|
|
||
| auto *Triple = LLVMGetDefaultTargetTriple(); |
There was a problem hiding this comment.
I'm seeing checks in the other unittest that checks if this is empty and bails if so, perhaps we also need it here? I think it's possible to not have a default target triple.
There was a problem hiding this comment.
Good point, done.
|
oh, also needs release notes in |
| @@ -0,0 +1,54 @@ | |||
| #include "llvm-c/Core.h" | |||
LLVMCreateTargetMachine takes a lot of parameters. This commit therefore introduces a dedicated option structure in the C interface, allowing the user to configure the various options and providing reasonable defaults. It also exposes the ABI option, which is used by a number of common backends, including ARM, MIPS, PPC, and RISCV. Exposing it via the C API makes it possible for users of those backends to configure the ABI without custom bindings.
sebastianpoeplau
force-pushed
the
add-llvm-create-target-machine-with-abi
branch
from
b9ed30e to
98b88de
Compare
|
lgtm! |
Thanks! I don't have permission to merge though - would you mind? 😊 |
|
Interesting, it seems that the changes are squashed and then committed with the PR description as the commit message 🧐 I'll know for next time that I need to keep the title and description up to date... Also, I've received an email reporting a build failure on NVPTX builders; it seems that the unit test needs to declare additional dependencies at least in that configuration. What's the process to get a fix in? Do I just create a follow-up PR? |
This patch adds missing dependencies required by the new unittest introduced by #68406.
|
@sebastianpoeplau I have the same problem with my local build. It should be fixed by dfcb890. |
Great, thanks for the fix @dtcxzyw! |
The ABI parameter is used by a number of common backends, including ARM, MIPS, PPC, and RISCV. Exposing it via the C API makes it possible for users of those backends to configure the ABI without custom bindings.