mips: set llvm_args -mno-check-zero-division for all mips targets#157873
Conversation
|
These commits modify compiler targets. |
|
r? @tiif rustbot has assigned @tiif. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Specifically https://doc.rust-lang.org/reference/expressions/operator-expr.html#r-expr.arith-logic.behavior documents:
Adding the flag does in fact have an effect on codegen (I'd have assumed that LLVM would be able to spot this, but I guess not) https://rust.godbolt.org/z/evacM46bM It's probably a good idea to capture that in an |
1ae5e5c to
12b004e
Compare
This comment has been minimized.
This comment has been minimized.
Added (but I'm unsure if I wrote it correctly, I haven't written a codegen test before). |
This comment has been minimized.
This comment has been minimized.
12b004e to
8bcda4f
Compare
This comment has been minimized.
This comment has been minimized.
8bcda4f to
59bab36
Compare
This comment has been minimized.
This comment has been minimized.
59bab36 to
d444395
Compare
| //@ assembly-output: emit-asm | ||
| //@ revisions: mips64el-unknown-linux-gnuabi64 | ||
| //@[mips64el-unknown-linux-gnuabi64] compile-flags: --target=mips64el-unknown-linux-gnuabi64 | ||
| //@[mips64el-unknown-linux-gnuabi64] needs-llvm-components: mips |
There was a problem hiding this comment.
| //@[mips64el-unknown-linux-gnuabi64] needs-llvm-components: mips | |
| //@ needs-llvm-components: mips |
All of these tests need mips, so I think you can factor this out.
There was a problem hiding this comment.
also the code's been written already anyway, but testing all targets doesn't add that much value. it would be more interesting to test the (new) default versus a configuration where you enable the div by zero trap explicitly.
There was a problem hiding this comment.
hmm guess not
tidy [target-specific-tests (tests)]: /checkout/tests/assembly-llvm/mips-div-no-trap.rs: revision mipsel-unknown-netbsd should specify `needs-llvm-components: mips` as it has `--target` set
tidy [target-specific-tests (tests)]: /checkout/tests/assembly-llvm/mips-div-no-trap.rs: revision mipsel-unknown-none should specify `needs-llvm-components: mips` as it has `--target` set
tidy [target-specific-tests (tests)]: /checkout/tests/assembly-llvm/mips-div-no-trap.rs: revision mipsisa32r6-unknown-linux-gnu should specify `needs-llvm-components: mips` as it has `--target` set
tidy [target-specific-tests (tests)]: /checkout/tests/assembly-llvm/mips-div-no-trap.rs: revision mipsisa32r6el-unknown-linux-gnu should specify `needs-llvm-components: mips` as it has `--target` set
tidy [target-specific-tests (tests)]: /checkout/tests/assembly-llvm/mips-div-no-trap.rs: revision mipsisa64r6-unknown-linux-gnuabi64 should specify `needs-llvm-components: mips` as it has `--target` set
tidy [target-specific-tests (tests)]: /checkout/tests/assembly-llvm/mips-div-no-trap.rs: revision mipsisa64r6el-unknown-linux-gnuabi64 should specify `needs-llvm-components: mips` as it has `--target` set
tidy [target-specific-tests (tests)]: FAIL
@jieyouxu is there a reason that needs-llvm-components is not "inherited" by revisions? e.g. codegen-flags is I believe.
There was a problem hiding this comment.
Not sure. Maybe when that was implemented it just wasn't "forwarded" for revisions?
| #![crate_type = "lib"] | ||
| #![feature(no_core, lang_items)] | ||
| #![no_core] | ||
|
|
There was a problem hiding this comment.
you can use minicore instead (there are plenty of examples of it in this directory) to remove a bunch of these manual language items.
| impl Div for i32 { | ||
| type Output = Self; | ||
|
|
||
| fn div(self, rhs: Self) -> Self { | ||
| self / rhs | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is a recursive definition? My godbolt uses the intrinsic manually instead.
There was a problem hiding this comment.
I think this is how a "language item" works and the real core uses the same pattern. But I'll use the intrinsic anyway to get rid all the panic_* language items.
|
Reminder, once the PR becomes ready for a review, use |
d444395 to
7fd6d53
Compare
This comment has been minimized.
This comment has been minimized.
7fd6d53 to
83d8c35
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
83d8c35 to
ce69624
Compare
This comment has been minimized.
This comment has been minimized.
ce69624 to
ed576bd
Compare
|
Huh, this is really weird. This really shouldn't be a backend option at all, the frontend (that is, Clang) should be inserting the trap if it wants to have it... |
…=folkertdev mips: set llvm_args -mno-check-zero-division for all mips targets In rust interger divide by zero is defined to panic, thus the inserted conditional trap should never trigger as the program should have panicked if the divisor is zero. So disable the insertion of the redundant conditional trap.
…=folkertdev mips: set llvm_args -mno-check-zero-division for all mips targets In rust interger divide by zero is defined to panic, thus the inserted conditional trap should never trigger as the program should have panicked if the divisor is zero. So disable the insertion of the redundant conditional trap.
…uwer Rollup of 10 pull requests Successful merges: - #155535 (export symbols: support macos/windows(32/64)) - #156538 (Refactor `AliasTy`. `AliasTerm` & `UnevaluatedConst` to use `Alias`) - #156807 (Add `T: PartialEq` bounds to derived `StructuralPartialEq` impls.) - #156950 (Staticlib rename internal symbols) - #157702 (Add expansion info to implied bounds) - #155616 (constify `TryFrom<Vec>` for array) - #156226 (diagnostics: point to coroutine body on higher-ranked auto trait errors) - #157873 (mips: set llvm_args -mno-check-zero-division for all mips targets) - #157953 (fix(rustc_codegen_ssa): Use cg_operand for Freeze check) - #157958 (doc: Document `-Zlint-rust-version`)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #157975 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#157975), which was unapproved. |
|
Ah, right, this is exactly the issue with the flag not being respected at So adding |
|
Hmm interesting... With -march=mips64el the flag is respected even at -O0. I suspect this is a llvm bug... |
|
It does look like an oversight, yeah. Anyhow, it is quite normal for LLVM tests to only consider If you do open an LLVM issue, please link back to this PR. I might be able to fix it. |
|
I'll update this PR to add -Copt-level=3 anyway. |
In rust interger divide by zero is defined to panic, thus the inserted conditional trap should never trigger as the program should have panicked if the divisor is zero. So disable the insertion of the redundant conditional trap.
ed576bd to
3410d7f
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. |
|
@bors try jobs=x86_64-gnu-nopt |
This comment has been minimized.
This comment has been minimized.
mips: set llvm_args -mno-check-zero-division for all mips targets try-job: x86_64-gnu-nopt
|
@bors r+ |
…uwer Rollup of 12 pull requests Successful merges: - #157788 (Accumulate multiple -Zsanitizer target modifiers) - #158012 (Stabilize `strip_circumfix`) - #157810 (Query the trait solver in slow path for `missing_debug_implementations`) - #157829 (tests: adapt two tests for LLVM 23 changes) - #157873 (mips: set llvm_args -mno-check-zero-division for all mips targets) - #157886 (Reject extra fields in MGCA struct const arguments) - #157917 (Don't suggest adding `in` to a `for` loop that already has one) - #157967 (Preserve track_caller for by-value dyn vtable shims) - #158019 (delegation: add simple test for incremental compilation) - #158023 (Move `UnusedDuplicate` diag struct to `rustc_attr_parsing`) - #158029 (Rename `project-stable-mir` to `project-rustc-public`) - #158044 (Add more tests for parallel frontend issues)
Rollup merge of #157873 - xry111:mips-no-div-by-zero-trap, r=folkertdev mips: set llvm_args -mno-check-zero-division for all mips targets In rust interger divide by zero is defined to panic, thus the inserted conditional trap should never trigger as the program should have panicked if the divisor is zero. So disable the insertion of the redundant conditional trap.
View all comments
In rust interger divide by zero is defined to panic, thus the inserted conditional trap should never trigger as the program should have panicked if the divisor is zero.
So disable the insertion of the redundant conditional trap.