Conversation
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I'll add tests for assembly output and for target incompatibility. |
This comment has been minimized.
This comment has been minimized.
|
|
||
| fn packed_stack_attr<'ll>(cx: &SimpleCx<'ll>, sess: &Session) -> Option<&'ll Attribute> { | ||
| if sess.target.arch != Arch::S390x { | ||
| return None; |
There was a problem hiding this comment.
Maybe panic here given that the frontend should already have emitted an error.
There was a problem hiding this comment.
hmm packed_stack_attr(...) is called every time llfn_attrs_from_instance is called indifferent of target arch.
And I think it would not be a good thing to add an arch check to llfn_attrs_from_instance as there are no other checks in that function. It blindly calls all possibilities and assumes that checks are already handled elsewhere.
5ddcabb to
4ebec9d
Compare
This comment has been minimized.
This comment has been minimized.
0d6d167 to
0c182af
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0c182af to
7fef6ea
Compare
|
This PR was rebased onto a different main 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. |
|
ok, i think most should be ok now. |
This comment has been minimized.
This comment has been minimized.
7fef6ea to
31a40dc
Compare
compiler/rustc_interface/src/util.rs
Outdated
| if sess.target.arch == Arch::S390x | ||
| && sess.opts.unstable_opts.packed_stack | ||
| && sess.unstable_target_features.contains(&Symbol::intern(&"backchain")) | ||
| && sess.target.abi != Abi::SoftFloat |
There was a problem hiding this comment.
target.abi is just in charge of cfg(target_abi), it doesn't control the actual ABI. Please check rustc_abi which controls the actual ABI.
There was a problem hiding this comment.
changed it to check on the soft-float unstable_target_feature which llvm will evaluate to switch abi for s390x.
| // this is the first place in the `run_compiler` flow where all this is available | ||
| if sess.target.arch == Arch::S390x | ||
| && sess.opts.unstable_opts.packed_stack | ||
| && sess.unstable_target_features.contains(&Symbol::intern(&"backchain")) |
There was a problem hiding this comment.
What happens if someone tries to enable backchain just for a particular function via #[target_feature] instead of -Ctarget-feature?
There was a problem hiding this comment.
ufff yes we have to check this for every call to llfn_attrs_from_instance at compiler runtime. is it ok to do ....contains(&Symbol::intern(... every time?
This comment has been minimized.
This comment has been minimized.
d886fbf to
f39c52b
Compare
this enables packed-stack just as -mpacked-stack in clang and gcc. packed-stack is needed on s390x for kernel development. Co-authored-by: Ralf Jung <post@ralfj.de>
f39c52b to
dc98326
Compare
this enables
-Zpacked-stackjust as-mpacked-stackin clang and gcc. packed-stack is needed on s390x for kernel development.For reference: #151154 and #150766
look at @uweigand s post for full explanation of what this does. Here a wrap-up:
#150766 (comment)