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.

Sign up for GitHub

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

Jump to bottom

adjust_abi: make fallback logic for ABIs a bit easier to read #137370

Merged
merged 1 commit into from
Feb 25, 2025
Merged

adjust_abi: make fallback logic for ABIs a bit easier to read #137370

merged 1 commit into from
Feb 25, 2025

Conversation

RalfJung
Copy link
Member

I feel like the match guards here make this unnecessarily harder to follow.

@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2025

r? @SparrowLii

rustbot has assigned @SparrowLii.
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 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 Feb 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2025

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

@RalfJung RalfJung changed the title adjust_abi: make fallback logic for x86 ABIs a bit easier to read adjust_abi: make fallback logic for ABIs a bit easier to read Feb 21, 2025
@RalfJung
Copy link
Member Author

While I'm touching this code anyway... @workingjubilee do you have any idea why this is not done as part of conv_from_spec_abi?

@workingjubilee
Copy link
Member

I intend to burn out the entire idea of "adjusting ABIs" as soon as I can check enough things off my list of things to clean up that it is on the top.

@workingjubilee
Copy link
Member

I don't really find this variant appreciably more legible.

@RalfJung
Copy link
Member Author

RalfJung commented Feb 21, 2025 •
edited
Loading

Hm... surprising.

The old logic makes it non-trivial to tell "what is all the logic that applies to Fastcall". I have to parse all match arms, see which ones contain Fastcall, extract their guards, and then re-assemble the corresponding if-then-else chain in my head. The new version makes that immediately obvious: if self.arch == "x86" { abi } else { C { unwind } }.

@workingjubilee
Copy link
Member

yes, I read things and organize them in my head in precisely the opposite way: when I see a match followed by if-else blocks inside the arms, I try to flatten them out into just their arms.

@RalfJung
Copy link
Member Author

But that's so many cases all mixed up without any structure? Many more degrees of freedom, many more different ways to write the same thing... and it's not even easy to visually identify the conditionals as the if <expr> drown in a sea of other symbols. I mean I guess that's how you work but I really can't put myself into that mindset even if I try hard, sorry.

Not sure how to resolve this, all I can say is it I feel I have to de-spaghetti this function every single time I look at it.^^

@workingjubilee
Copy link
Member

🤔 It's a very simple structure, isn't it? An array.

SparrowLii
SparrowLii reviewed Feb 22, 2025
View reviewed changes
compiler/rustc_target/src/spec/mod.rs
} else {
C { unwind }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

TBH I prefer match (abi, &self.arch) { ... }

Copy link
Member

@SparrowLii SparrowLii Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's hard to judge the pros and cons of these writings. I gonna merge this PR if you don't make more changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work for System where the logic depends on more than just the architecture.

@RalfJung
Copy link
Member Author

🤔 It's a very simple structure, isn't it? An array.

Exactly. The match makes it a flat list without further structure. But the code does have further structure, it's a decision tree. So writing it as a flat list obscures the underlying structure.

@workingjubilee
Copy link
Member

Well, it kinda shouldn't, honestly. Even the MSVC compiler prefers to reject vectorcall more often:

On ARM machines, __vectorcall is accepted and ignored by the compiler. On ARM64EC, __vectorcall is unsupported and rejected by the compiler.

Since we're requiring sse2, we should probably just reject it.

@RalfJung
Copy link
Member Author

Not sure what exactly you are replying to, but this PR is not intended to change behavior. I agree vectorcall should be rejected on non-x86 targets, but it seems odd to do this one target at a time so see #137018 for that.

SparrowLii
SparrowLii approved these changes Feb 25, 2025 •
edited
Loading
View reviewed changes
@SparrowLii
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 25, 2025

📌 Commit f7ae9e1 has been approved by SparrowLii

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 Feb 25, 2025
@fmease fmease mentioned this pull request Feb 25, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2025
@bors
Auto merge of rust-lang#137608 - fmease:rollup-h4siso6, r=fmease
85abb27 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#137370 (adjust_abi: make fallback logic for ABIs a bit easier to read)
 - rust-lang#137444 (Improve behavior of `IF_LET_RESCOPE` around temporaries and place expressions)
 - rust-lang#137464 (Fix invalid suggestion from type error for derive macro)
 - rust-lang#137539 ( Add rustdoc-gui regression test for rust-lang#137082 )
 - rust-lang#137576 (Don't doc-comment BTreeMap<K, SetValZST, A>)
 - rust-lang#137595 (remove `simd_fpow` and `simd_fpowi`)
 - rust-lang#137600 (type_ir: remove redundant part of comment)
 - rust-lang#137602 (feature: fix typo in attribute description)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e02de83 into rust-lang:master Feb 25, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 25, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2025
@rust-timer
Unrolled build for rust-lang#137370
03f0c93 
Rollup merge of rust-lang#137370 - RalfJung:x86-abi-fallback, r=SparrowLii

adjust_abi: make fallback logic for ABIs a bit easier to read

I feel like the match guards here make this unnecessarily harder to follow.
@RalfJung RalfJung deleted the x86-abi-fallback branch February 26, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SparrowLii SparrowLii SparrowLii approved these changes

Assignees

@SparrowLii SparrowLii

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
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
5 participants
@RalfJung @rustbot @workingjubilee @SparrowLii @bors