Skip to content

rustc_target: callconv: mips64: Return structs with single f128 in FPRs#159967

Open
Gelbpunkt wants to merge 1 commit into
rust-lang:mainfrom
Gelbpunkt:mips64-struct-return-single-f128
Open

rustc_target: callconv: mips64: Return structs with single f128 in FPRs#159967
Gelbpunkt wants to merge 1 commit into
rust-lang:mainfrom
Gelbpunkt:mips64-struct-return-single-f128

Conversation

@Gelbpunkt

Copy link
Copy Markdown
Contributor

The MIPS n64 ABI returns structs that meet the following requirements:

  • Up to 128 bits large
  • Only one or two fields, all of which are floating point
  • Offset of the first field is zero

in floating point registers. This was already accounted for, but the edge case of a struct with a single f128 field was not handled correctly and would always be returned in integer registers.

If we tell the backend to use a f128 register here, LLVM will return the value in two of the 64-bit FPRs, $f0 and $f2. That is equivalent to how Clang, GCC and also Rustc today would return a long double / f128.

However, both Clang and GCC return a struct with a single long double field in $f0 and $f1. In order to achieve the same result in Rustc, we need to set the InReg attribute on the return value.

With this, we now match Clang and GCC.

Clang: https://godbolt.org/z/za8qv9P4n
GCC: https://godbolt.org/z/9qcdGsce6
Rustc currently: https://godbolt.org/z/8sP5G4ash
LLVM IR comparisons: https://godbolt.org/z/ojaTTY9hW


I'm not sure if it is acceptable to use Reg::f128 since MIPS has no 128-bit floating point registers unless MSA is present. LLVM lowers it correctly to use two FPRs, not sure if this would cause issues with GCC.

f16 probably also needs some changes here, but I'll do that as a follow-up.


r? @folkertdev

and Folkert also told me to cc @tgross35 :)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2026
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 26, 2026

@folkertdev folkertdev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this (wrapped) float logic is quite subtle, can you add an assembly test for it? tests/assembly-llvm/mips-div-no-trap.rs can serve as a template, though just 2 revisions (BE and LE) should be fine.

View changes since this review

Comment thread compiler/rustc_target/src/callconv/mips64.rs Outdated
Comment thread compiler/rustc_target/src/callconv/mips64.rs
@Gelbpunkt
Gelbpunkt requested a review from folkertdev July 26, 2026 14:46
ret.cast_to(reg);
// The inreg attribute forces LLVM to return a struct containing a f128 in
// $f0 and $f1 rather than $f0 and $f2, see:
// https://github.com/llvm/llvm-project/blob/a81db64570f94c2ca8ac0f598c0b5bba1a7ae59e/llvm/lib/Target/Mips/MipsCallingConv.td#L48-L51

@folkertdev folkertdev Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  // Contrary to the ABI documentation, a struct containing a long double is
  // returned in $f0, and $f1 instead of the usual $f0, and $f2. This is to
  // match the de facto ABI as implemented by GCC.

lol classic

View changes since the review

Comment thread tests/assembly-llvm/mips-struct-f128.rs Outdated

@folkertdev folkertdev Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this could be a bit more general, so maybe mips-struct-float and testing some of the other 1 and 2 element cases too?

For more popular targets we'd find ABI issues organically, but for targets like mips it's possible something would slip through.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it so we test all the combinations of f32 / f64 / f128 that should be returned in FPRs

@Gelbpunkt
Gelbpunkt force-pushed the mips64-struct-return-single-f128 branch 2 times, most recently from dccc817 to ba8c047 Compare July 26, 2026 16:32
The MIPS n64 ABI returns structs that meet the following requirements:

- Up to 128 bits large
- Only one or two fields, all of which are floating point
- Offset of the first field is zero

in floating point registers. This was already accounted for, but the
edge case of a struct with a single f128 field was not handled correctly
and would always be returned in integer registers.

If we tell the backend to use a f128 register here, LLVM will return the
value in two of the 64-bit FPRs, $f0 and $f2. That is equivalent to
how Clang, GCC and also Rustc today would return a long double / f128.

However, both Clang and GCC return a struct with a single long double
field in $f0 and $f1. In order to achieve the same result in Rustc, we
need to set the InReg attribute on the return value.

With this, we now match Clang and GCC.

Clang: https://godbolt.org/z/za8qv9P4n
GCC: https://godbolt.org/z/9qcdGsce6
Rustc currently: https://godbolt.org/z/8sP5G4ash
LLVM IR comparisons: https://godbolt.org/z/ojaTTY9hW
@Gelbpunkt
Gelbpunkt force-pushed the mips64-struct-return-single-f128 branch from ba8c047 to b0bc5a0 Compare July 26, 2026 16:33
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
..................F.........................       (144/144)

======== tests/rustdoc-gui/source-code-wrapping.goml ========

[ERROR] source-code-wrapping output:
Runtime.callFunctionOn timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.
stack: ProtocolError: Runtime.callFunctionOn timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.
    at <instance_members_initializer> (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/common/CallbackRegistry.js:102:14)
    at new Callback (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/common/CallbackRegistry.js:106:16)
    at CallbackRegistry.create (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/common/CallbackRegistry.js:25:26)
    at Connection._rawSend (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/cdp/Connection.js:118:26)
    at CdpCDPSession.send (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/cdp/CdpSession.js:72:14)
    at #evaluate (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/cdp/ExecutionContext.js:360:50)
    at ExecutionContext.evaluate (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/cdp/ExecutionContext.js:274:36)
    at IsolatedWorld.evaluate (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/cdp/IsolatedWorld.js:102:30)
    at CdpFrame.evaluate (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/api/Frame.js:359:43)
    at CdpFrame.<anonymous> (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/puppeteer/util/decorators.js:101:27)



<= doc-ui tests done: 143 succeeded, 1 failed, 0 filtered out

@folkertdev folkertdev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rust-bors

rust-bors Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

📌 Commit b0bc5a0 has been approved by folkertdev

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

Reason for tree closure: spurious failures

@rust-bors rust-bors Bot 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 Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

4 participants