Skip to content

Conversation

@a4lg
Copy link
Contributor

@a4lg a4lg commented Sep 14, 2025

Despite that the fflags register (representing floating point exception flags) is stated as a flag register in the reference, it's not
in the default clobber list of the RISC-V inline assembly and it would be better to fix it.

Related

@rustbot label +O-riscv

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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 Sep 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 14, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the O-riscv Target: RISC-V architecture label Sep 14, 2025
@a4lg
Copy link
Contributor Author

a4lg commented Sep 14, 2025

"Against the reference" might be an exaggeration. Still, I think they should be added to the default clobber list.

@a4lg a4lg force-pushed the riscv-inline-asm-default-clobber-float-flags branch from f32cd1d to 3c92fa1 Compare September 14, 2025 01:50
@a4lg a4lg changed the title RISC-V: Add floating point state registers to inline assembly's clobber list rustc_codegen_llvm: Add floating point state registers to RISC-V inline assembly's clobber list Sep 14, 2025
@jieyouxu
Copy link
Member

r? compiler

@rustbot rustbot assigned nnethercote and unassigned jieyouxu Sep 14, 2025
@nnethercote
Copy link
Contributor

nnethercote commented Sep 14, 2025

"Against the reference" might be an exaggeration

I'm having trouble understanding what "Against the reference" means here. Do you mean that the reference is incorrect? Or the reference is correct but the code currently does something different?

@taiki-e: I know nothing about RISC-V. You wrote tests/codegen/asm/riscv-clobbers.rs. Does this look ok to you?

@taiki-e
Copy link
Member

taiki-e commented Sep 14, 2025

cc @Amanieu

@a4lg
Copy link
Contributor Author

a4lg commented Sep 15, 2025

@nnethercote

I'll rewrite the commit message and I'll write a part of the draft v2 below (for clarity).

Despite that the fflags register is stated as a flag register in the reference, it's not in the default clobber list of the RISC-V inline assembly and it would be better to fix it. (cont.)

@a4lg
Copy link
Contributor Author

a4lg commented Sep 15, 2025

I'm investigating the LLVM source code for interactions between subset/superset registers (e.g. fflags is a readable+writable mirror of certain bits of fcsr) and will adjust the clobber list based on the results.

About adding frm to the list

If we don't have enough benefits having that, we have an option NOT to do it without causing any real problems (since this change will be backward-compatible and the only reason I initially added frm to the clobber list in this PR is for consistency).

About removing vxrm from the list

If we do that, we technically make a backward-incompatible change.
Still, vxrm is only used by RISC-V vectors (RVV; based on dynamically-sized vectors like Arm SVE) and we are not yet ready for the native support for such vectors. So, removing vxrm from the clobber list will be a safe option until we start actively supporting RVV through intrinsics and vector types.

Despite that the `fflags` register (representing floating point
exception flags) is stated as a flag register in the reference, it's not
in the default clobber list of the RISC-V inline assembly and it would
be better to fix it.
@a4lg a4lg changed the title rustc_codegen_llvm: Add floating point state registers to RISC-V inline assembly's clobber list rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list Sep 15, 2025
@a4lg a4lg force-pushed the riscv-inline-asm-default-clobber-float-flags branch from 3c92fa1 to 5ebdec5 Compare September 15, 2025 02:17
@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@a4lg
Copy link
Contributor Author

a4lg commented Sep 15, 2025

Removed frm from the clobber list and overhauled the commit message.

@a4lg
Copy link
Contributor Author

a4lg commented Sep 15, 2025

Looking at the LLVM source code, relations between fcsr and its subset mirrors fflags and frm are defined internally but ones between vcsr and its subset mirrors vxsat and vxrm are not defined.

But it seems at least Clang does not support clobbering vcsr (due to limited set of registers handled by Clang's inline assembly handling code). I'm checking whether LLVM supports clobbering it.

@Amanieu
Copy link
Member

Amanieu commented Sep 15, 2025

We don't need to clobber vcsr since we already clobber vxrm and vxsat separately. There is nothing else in vcsr.

@Amanieu
Copy link
Member

Amanieu commented Sep 15, 2025

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 15, 2025

📌 Commit 5ebdec5 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2025
@a4lg
Copy link
Contributor Author

a4lg commented Sep 15, 2025

I'm not yet sure (e.g. what will happen if vxsat is considered changed and vcsr is read) but at least I can accept that the current PR implementation is as safe as Clang. So let's go on (and fix later if necessary).

Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 15, 2025
…ber-float-flags, r=Amanieu

rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list

Despite that the `fflags` register (representing floating point exception flags) is stated as a flag register [in the reference](https://doc.rust-lang.org/reference/inline-assembly.html#r-asm.rules.preserved-registers), it's not
in the default clobber list of the RISC-V inline assembly and it would be better to fix it.
bors added a commit that referenced this pull request Sep 15, 2025
Rollup of 8 pull requests

Successful merges:

 - #146402 (interpret: fix overlapping aggregate initialization)
 - #146530 (rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list)
 - #146533 (Note some previous attempts to change the Default impl for `[T; 0]`)
 - #146539 (fix 404 MCP link)
 - #146546 (Switch `std::vec::PeekMut::pop` from self to this parameter.)
 - #146549 (On FreeBSD, use readdir instead of readdir_r)
 - #146559 (Fix typo in error message)
 - #146563 (bootstrap.py: disable incremental build for bootstrap in CI)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Sep 15, 2025
Rollup of 8 pull requests

Successful merges:

 - #146402 (interpret: fix overlapping aggregate initialization)
 - #146530 (rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list)
 - #146533 (Note some previous attempts to change the Default impl for `[T; 0]`)
 - #146539 (fix 404 MCP link)
 - #146546 (Switch `std::vec::PeekMut::pop` from self to this parameter.)
 - #146549 (On FreeBSD, use readdir instead of readdir_r)
 - #146559 (Fix typo in error message)
 - #146563 (bootstrap.py: disable incremental build for bootstrap in CI)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Sep 15, 2025
Rollup of 8 pull requests

Successful merges:

 - #146402 (interpret: fix overlapping aggregate initialization)
 - #146530 (rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list)
 - #146533 (Note some previous attempts to change the Default impl for `[T; 0]`)
 - #146539 (fix 404 MCP link)
 - #146546 (Switch `std::vec::PeekMut::pop` from self to this parameter.)
 - #146549 (On FreeBSD, use readdir instead of readdir_r)
 - #146559 (Fix typo in error message)
 - #146563 (bootstrap.py: disable incremental build for bootstrap in CI)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 15, 2025
…ber-float-flags, r=Amanieu

rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list

Despite that the `fflags` register (representing floating point exception flags) is stated as a flag register [in the reference](https://doc.rust-lang.org/reference/inline-assembly.html#r-asm.rules.preserved-registers), it's not
in the default clobber list of the RISC-V inline assembly and it would be better to fix it.
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 15, 2025
…ber-float-flags, r=Amanieu

rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list

Despite that the `fflags` register (representing floating point exception flags) is stated as a flag register [in the reference](https://doc.rust-lang.org/reference/inline-assembly.html#r-asm.rules.preserved-registers), it's not
in the default clobber list of the RISC-V inline assembly and it would be better to fix it.
bors added a commit that referenced this pull request Sep 15, 2025
Rollup of 12 pull requests

Successful merges:

 - #146338 (Extends AArch64 branch protection support to include GCS)
 - #146344 (tests/codegen-llvm: Make rust-abi-arch-specific-adjustment portable)
 - #146402 (interpret: fix overlapping aggregate initialization)
 - #146405 (Add relnotes for 1.90.0)
 - #146530 (rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list)
 - #146533 (Note some previous attempts to change the Default impl for `[T; 0]`)
 - #146539 (fix 404 MCP link)
 - #146546 (Switch `std::vec::PeekMut::pop` from self to this parameter.)
 - #146549 (On FreeBSD, use readdir instead of readdir_r)
 - #146559 (Fix typo in error message)
 - #146563 (bootstrap.py: disable incremental build for bootstrap in CI)
 - #146576 (opt-dist: don't set `RUST_LOG=collector=debug`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 15, 2025
…ber-float-flags, r=Amanieu

rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list

Despite that the `fflags` register (representing floating point exception flags) is stated as a flag register [in the reference](https://doc.rust-lang.org/reference/inline-assembly.html#r-asm.rules.preserved-registers), it's not
in the default clobber list of the RISC-V inline assembly and it would be better to fix it.
bors added a commit that referenced this pull request Sep 15, 2025
Rollup of 9 pull requests

Successful merges:

 - #146344 (tests/codegen-llvm: Make rust-abi-arch-specific-adjustment portable)
 - #146530 (rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list)
 - #146533 (Note some previous attempts to change the Default impl for `[T; 0]`)
 - #146539 (fix 404 MCP link)
 - #146546 (Switch `std::vec::PeekMut::pop` from self to this parameter.)
 - #146549 (On FreeBSD, use readdir instead of readdir_r)
 - #146559 (Fix typo in error message)
 - #146563 (bootstrap.py: disable incremental build for bootstrap in CI)
 - #146576 (opt-dist: don't set `RUST_LOG=collector=debug`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f34e30a into rust-lang:master Sep 16, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 16, 2025
rust-timer added a commit that referenced this pull request Sep 16, 2025
Rollup merge of #146530 - a4lg:riscv-inline-asm-default-clobber-float-flags, r=Amanieu

rustc_codegen_llvm: Adjust RISC-V inline assembly's clobber list

Despite that the `fflags` register (representing floating point exception flags) is stated as a flag register [in the reference](https://doc.rust-lang.org/reference/inline-assembly.html#r-asm.rules.preserved-registers), it's not
in the default clobber list of the RISC-V inline assembly and it would be better to fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-riscv Target: RISC-V architecture S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

7 participants