Skip to content
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

Use bitand when checking for signed integer division overflow #89459

Merged
merged 1 commit into from
Oct 3, 2021

Conversation

tspiteri
Copy link
Contributor

@tspiteri tspiteri commented Oct 2, 2021

For self == Self::MIN && rhs == -1, LLVM does not realize that this is the same check made by self / rhs, so the code generated may have some unnecessary duplication. For (self == Self::MIN) & (rhs == -1), LLVM realizes it is the same check.

For `self == Self::MIN && rhs == -1`, LLVM does not realize that this is the
same check made by `self / rhs`, so the code generated may have some unnecessary
duplication. For `(self == Self::MIN) & (rhs == -1)`, LLVM realizes it is the
same check.
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2021
@tspiteri
Copy link
Contributor Author

tspiteri commented Oct 2, 2021

An example for checked_div_euclid shows the better code generation.

Note that this PR does not affect checked_div and checked_rem, since they make use of unchecked_div and unchecked_rem which do not have any checks, so there is already no duplication. I included the changes in the two methods for internal consistency in the file.

@wooster0
Copy link
Contributor

wooster0 commented Oct 2, 2021

I really think this is something that should be improved at the code generation level instead of doing this manually for every case like this because there are likely a lot more cases and doing so would be cleaner and could cover more cases IMO.

@kennytm
Copy link
Member

kennytm commented Oct 3, 2021

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Oct 3, 2021

📌 Commit 1139ee3 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2021
@bors
Copy link
Contributor

bors commented Oct 3, 2021

⌛ Testing commit 1139ee3 with merge 4479cb8...

@bors
Copy link
Contributor

bors commented Oct 3, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 4479cb8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2021
@bors bors merged commit 4479cb8 into rust-lang:master Oct 3, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 3, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4479cb8): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@tspiteri tspiteri deleted the idiv-overflow-bitand branch October 5, 2021 12:52
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2021
Replace some operators in libcore with their short-circuiting equivalents

In libcore there are a few occurrences of bitwise operators used in boolean expressions instead of their short-circuiting equivalents. This makes it harder to perform some kinds of source code analysis over libcore, for example [MC/DC] code coverage (a requirement in safety-critical environments).

This PR aims to remove as many bitwise operators in boolean expressions from libcore as possible, without any performance regression and without other changes. This means not all bitwise operators are removed, only the ones that don't have any difference with their short-circuiting counterparts. This already simplifies achieving MC/DC coverage, and the other functions can be changed in future PRs.

The PR is best reviewed commit-by-commit, and each commit has the resulting assembly in the message.

## Checked integer methods

These methods recently switched to bitwise operators in PRs rust-lang#89459 and rust-lang#89351. I confirmed bitwise operators are needed in most of the functions, except these two:

* `{integer}::checked_div` ([Godbolt link (nightly)](https://rust.godbolt.org/z/17efh5jPc))
* `{integer}::checked_rem` ([Godbolt link (nightly)](https://rust.godbolt.org/z/85qGWc94K))

`@tspiteri` already mentioned this was the case in rust-lang#89459 (comment), but opted to also switch those two to bitwise operators for consistency. As that makes MC/DC analysis harder this PR proposes switching those two back to short-circuiting operators.

## `{unsigned_ints}::carrying_add`

[Godbolt link (1.56.0)](https://rust.godbolt.org/z/vG9vx8x48)

In this instance replacing the `|` with `||` produces the exact same assembly when optimizations are enabled, so switching to the short-circuiting operator shouldn't have any impact.

## `{unsigned_ints}::borrowing_sub`

[Godbolt link (1.56.0)](https://rust.godbolt.org/z/asEfKaGE4)

In this instance replacing the `|` with `||` produces the exact same assembly when optimizations are enabled, so switching to the short-circuiting operator shouldn't have any impact.

## String UTF-8 validation

[Godbolt link (1.56.0)](https://rust.godbolt.org/z/a4rEbTvvx)

In this instance replacing the `|` with `||` produces practically the same assembly, with the two operands for the "or" swapped:

```asm
; Old
mov  rax, qword ptr [rdi + rdx + 8]
or   rax, qword ptr [rdi + rdx]
test rax, r9
je   .LBB0_7

; New
mov  rax, qword ptr [rdi + rdx]
or   rax, qword ptr [rdi + rdx + 8]
test rax, r8
je   .LBB0_7
```

[MC/DC]: https://en.wikipedia.org/wiki/Modified_condition/decision_coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants