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.
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.
There was a problem hiding this comment.
I think checking rustc_abi makes most sense.
compiler/rustc_interface/src/util.rs
Outdated
| // 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?
There was a problem hiding this comment.
from_target_feature_attr is probably a better, higher-level place to check this?
You can add backchain as a pre-interned symbol in compiler/rustc_span/src/symbol.rs so you don't have to re-intern it every time.
There was a problem hiding this comment.
I am back on working on this. yes in from_target_feature_attr we can check for the #[target_feature(...)] attributes. BUT unfortunately not for the -Ctarget-feature... attributes ..
There was a problem hiding this comment.
Yeah the handling for -Ctarget-feature and #[target_feature] is entirely separate. Maybe it's possible to introduce a new helper that gets called form those 2 places and that performs these kinds of consistency checks?
There was a problem hiding this comment.
Ah I see you're now doing the check in packed_stack_attr. That also seems reasonable.
This comment has been minimized.
This comment has been minimized.
d886fbf to
f39c52b
Compare
f39c52b to
dc98326
Compare
|
looks good for me, but it's better to be reviewed by someone with related domain knowledge. |
This comment has been minimized.
This comment has been minimized.
c490b46 to
ebb7f42
Compare
This comment has been minimized.
This comment has been minimized.
|
hold on I messed up that test with the last commit. I'll fix that tomorrow morning. |
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>
ebb7f42 to
6753aec
Compare
|
ok I think its ready now for next review round. @rustbot ready |
| return None; | ||
| } | ||
|
|
||
| // The backchain and softfloat flags can be set via -Ctarget-features=... |
There was a problem hiding this comment.
I'd suggest an early return on !sess.opts.unstable_opts.packed_stack to avoid doing any of this work if we don't have to.
removed comment that was not needed in the end
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)