-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Enable stack probes on aarch64 for LLVM 18 #118491
Conversation
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
These commits modify compiler targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked closely at the LLVM changes but is it possible to enable this for aarch64-pc-windows-msvc
as well?
I was assuming that Windows targets would already (implicitly) be using |
Yeah, it looks like Windows has had aarch64 probing for years: (through the external call mandated by the platform, not inline codegen) |
Cg_clif always uses inline stack probes on aarch64. Even on Windows. Is there any documentation about why windows requires external stack probes? |
I may be overstating it to call As noted there, part of the effect of that call is for stack growth. As long as that's an indirect effect -- probing the guard page to trigger growth -- then cg_clif's inline probes should be fine for that as well. |
Yeah, inline stack probing support was added to cranelift for that stack growing on windows. The way you worded it, it seemed like on windows stack probes are required to be outline stack probes. |
☔ The latest upstream changes (presumably #116565) made this pull request unmergeable. Please resolve the merge conflicts. |
fd00a51
to
b99b5e5
Compare
@@ -18,6 +18,7 @@ pub fn target() -> Target { | |||
options: TargetOptions { | |||
features: "+neon,+fp-armv8,+apple-a7".into(), | |||
max_atomic_width: Some(128), | |||
stack_probes: StackProbeType::Inline, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to gate use of StackProbeType::Inline
on LLVM 18 or newer? Is this just ignored for unsupported targets on earlier versions of LLVM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ignored -- or rather goes unchecked and unnoticed. The other times we've concerned ourselves about the version is when we know there are bugs in the implementation.
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (e6d1b0e): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 673.133s -> 673.665s (0.08%) |
…, r=workingjubilee Remove outdated footnote "missing-stack-probe" in platform-support ... after rust-lang#120055 and rust-lang#118491. cc rust-lang#77071 (comment).
Rollup merge of rust-lang#122094 - slanterns:arm-stack-probe-footnote, r=workingjubilee Remove outdated footnote "missing-stack-probe" in platform-support ... after rust-lang#120055 and rust-lang#118491. cc rust-lang#77071 (comment).
I tested this on
aarch64-unknown-linux-gnu
with LLVM main (~18).cc #77071, to be closed once we upgrade our LLVM submodule.