Skip to content
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

Always specify llvm_abiname for RISC-V targets #131807

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

beetrees
Copy link
Contributor

For RISC-V targets, when llvm_abiname is not specified LLVM will infer the ABI from the target features, causing #116344 to occur. This PR adds the correct llvm_abiname to all RISC-V targets where it is missing (which are all soft-float targets), and adds a test to prevent future RISC-V targets from accidentally omitting llvm_abiname. The only affect of this PR is that -Ctarget-feature=+f (or similar) will no longer affect the ABI on the modified targets.

r? @RalfJung

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

I can't speak to the consequences of this PR, so I can't review it, sorry. All I am doing is going around and bothering everyone to get their ABIs in check, I don't actually know the technical details of how exactly to do that on each architecture. ;)

r? compiler

@rustbot rustbot assigned nnethercote and unassigned RalfJung Oct 17, 2024
@RalfJung
Copy link
Member

The only affect of this PR is that -Ctarget-feature=+f (or similar) will no longer affect the ABI on the modified targets.

That sounds great!
What will -Ctarget-feature=-f do when the abiname says "use floats"?

@workingjubilee
Copy link
Member

What will -Ctarget-feature=-f do when the abiname says "use floats"?

I think the answer is actually, surprisingly, that LLVM shrieks "DON'T!" at us. Obviously we have to actually reach codegen, but LLVM actually checks this sort of dependency for the RISCV backend.

@RalfJung
Copy link
Member

RalfJung commented Oct 17, 2024

I wish that would be true. However, according to this, the answer is "Disabling target features required by the requested ABI will cause LLVM to ignore the ABI", which sounds less nice.

@RalfJung
Copy link
Member

Also we probably want to show a nicer error than have LLVM do error reporting. But it's always good to have a safety net :)

@workingjubilee
Copy link
Member

Ah, yeah, I checked and the code that I thought might be there isn't. Sadness!

@RalfJung
Copy link
Member

RalfJung commented Oct 17, 2024 via email

@RalfJung
Copy link
Member

RalfJung commented Oct 17, 2024 via email

@workingjubilee
Copy link
Member

I think we can make that happen, yeah. RISCV does some forms of feature dependency-checking already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants