Repository navigation
Re-enable extension of bool return values on x86_64 - #163917
Conversation
|
|
|
is this worth a stable/beta backport? |
Nominating just in case. |
39167e7 to
61f40b0
Compare
|
beta backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels are handled by them. |
|
stable backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels are handled by them. |
|
I have approved the backports without this PR being yet properly reviewed and merged (sorryu about that) r? compiler |
|
A random reroll here is likely unhelpful, ABI handling experts are scarce. I'll ask in the private channel. I'll do a review pass as well. |
There was a problem hiding this comment.
Change itself looks right to me, at least from what I can gather. Some non-blocking nits.
@rustbot author
| // ZERO/SIGN-EXTENDING TO 32 BITS NON-EXTENDING | ||
| // ============================== ======================= | ||
| // x86_64: void @c_arg_bool(i1 zeroext %_a) |
There was a problem hiding this comment.
I also noticed that because of how the test is written, if codegen starts adding a zeroext on a return type where it is wrong to do so, the test will pass anyway.
Yeah, that seems a bit awkward since it defeats the purpose of the return checks...
For the non-extending return check cases, can we instead match via patterns such as
// x86_64-linux: define {{(dso_local )?}}i1 @c_ret_bool()That way if the return type gets a zeroext this check should fail. Using this pattern against the uefi one (where it does zeroext) indeed fails to match.
(This is not blocking; I think we can also do this as a follow-up, since this is pre-existing.)
There was a problem hiding this comment.
Assuming we have the option to pass FileCheck arguments, you could also use something like --implicit-check-not=zeroext --implicit-check-not=signext.
There was a problem hiding this comment.
Hm yeah. I'm not sure using the implicit flags is much better though, that also seems not very straightforward when looking at the actual diff right?
61f40b0 to
3d42c2c
Compare
|
r? @nikic |
3d42c2c to
a0f1885
Compare
|
@bors r+ |
Rollup merge of #163917 - saethlin:bool-return-extend, r=nikic Re-enable extension of bool return values on x86_64 I think this fixes #163911 Though the test here is a bit hard to figure out, I'm not sure under what conditions expectation goes in the left or the right column. Do I decide that per-target or per-instance? Because x86_64 seems obligated to extend bool returns, so whether it is a "target that extends" or not depends on the type we're dealing with. I also noticed that because of how the test is written, if codegen starts adding a `zeroext` on a return type where it is wrong to do so, the test will pass anyway.
[beta] backports - Fix ICE for ambiguous candidates on method probing #162904 - Don't imply incorrect things about `Global` in the docs of `System` #163342 - make semicolon_in_expressions_from_non_local_macros not report-in-deps #163364 - Force the correct type variable to never for method resolution on an adjusted never type #163450 - Update LLVM submodule to latest release/23.x branch #163615 - Switch TLS implementation for wasi and bump SDK version to 34 #163809 - use the type name instead of `Self` in constructor paths again #163863 - Re-enable extension of bool return values on x86_64 #163917 - Add `synchronization` import lib to windows-gnullvm dist #163933 - fix(bootstrap/darwin): fix rpath for distributed LLD #163954 r? me
…saethlin Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs` Follow-up to rust-lang#163917. * Fix some misplaced patterns in extending vs non-extending columns. * Tighten return attribute patterns to actually reject unexpected `zeroext`/`signext` attributes. * See rust-lang#163917 (comment) r? @saethlin (or compiler)
…saethlin Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs` Follow-up to rust-lang#163917. * Fix some misplaced patterns in extending vs non-extending columns. * Tighten return attribute patterns to actually reject unexpected `zeroext`/`signext` attributes. * See rust-lang#163917 (comment) r? @saethlin (or compiler)
…saethlin Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs` Follow-up to rust-lang#163917. * Fix some misplaced patterns in extending vs non-extending columns. * Tighten return attribute patterns to actually reject unexpected `zeroext`/`signext` attributes. * See rust-lang#163917 (comment) r? @saethlin (or compiler)
…saethlin Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs` Follow-up to rust-lang#163917. * Fix some misplaced patterns in extending vs non-extending columns. * Tighten return attribute patterns to actually reject unexpected `zeroext`/`signext` attributes. * See rust-lang#163917 (comment) r? @saethlin (or compiler)
…saethlin Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs` Follow-up to rust-lang#163917. * Fix some misplaced patterns in extending vs non-extending columns. * Tighten return attribute patterns to actually reject unexpected `zeroext`/`signext` attributes. * See rust-lang#163917 (comment) r? @saethlin (or compiler)
Rollup merge of #164016 - jieyouxu:jieyouxu/tighten-test, r=saethlin Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs` Follow-up to #163917. * Fix some misplaced patterns in extending vs non-extending columns. * Tighten return attribute patterns to actually reject unexpected `zeroext`/`signext` attributes. * See #163917 (comment) r? @saethlin (or compiler)
View all comments
I think this fixes #163911
Though the test here is a bit hard to figure out, I'm not sure under what conditions expectation goes in the left or the right column. Do I decide that per-target or per-instance? Because x86_64 seems obligated to extend bool returns, so whether it is a "target that extends" or not depends on the type we're dealing with.
I also noticed that because of how the test is written, if codegen starts adding a
zeroexton a return type where it is wrong to do so, the test will pass anyway.