Skip to content

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Sep 30, 2022

The known problems with x86 inline-asm stack probes have been solved on LLVM main (16), so this flips the switch. Anyone using bleeding-edge LLVM with rustc can start testing this, as I have done locally. We'll get more direct rust-ci when LLVM 16 branches and we start our upgrade, and we can always patch or disable it then if we find new problems.

The previous attempt was #77885, reverted in #84708.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 30, 2022
@rust-highfive
Copy link
Contributor

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Contributor

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2022
@cuviper
Copy link
Member Author

cuviper commented Sep 30, 2022

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned petrochenkov Sep 30, 2022
@nagisa
Copy link
Member

nagisa commented Oct 1, 2022

You mention all the known issues. I wonder if it would be worthwhile to reference the known ones here?

@bors r+ either way.

@bors
Copy link
Collaborator

bors commented Oct 1, 2022

📌 Commit ed9e6f2 has been approved by nagisa

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 Oct 1, 2022
@@ -635,6 +635,10 @@ pub enum StackProbeType {
}

impl StackProbeType {
// LLVM X86 targets (ix86 and x86_64) can use inline-asm stack probes starting with LLVM 16.
// Notable past issues were rust#83139 (fixed in 14) and rust#84667 (fixed in 16).
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the reference I made to known issues, which were also mentioned in the distributed comments that I removed.

@bors
Copy link
Collaborator

bors commented Oct 3, 2022

⌛ Testing commit ed9e6f2 with merge 607b829...

@bors
Copy link
Collaborator

bors commented Oct 3, 2022

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 607b829 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2022
@bors bors merged commit 607b829 into rust-lang:master Oct 3, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (607b829): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.8% [4.6%, 5.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@cuviper cuviper deleted the x86-stack-probes branch October 15, 2022 00:13
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Enable inline stack probes on X86 with LLVM 16

The known problems with x86 inline-asm stack probes have been solved on LLVM main (16), so this flips the switch. Anyone using bleeding-edge LLVM with rustc can start testing this, as I have done locally. We'll get more direct rust-ci when LLVM 16 branches and we start our upgrade, and we can always patch or disable it then if we find new problems.

The previous attempt was rust-lang#77885, reverted in rust-lang#84708.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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