Eliminate some other bound checks when index comes from an enum #75529

bugadani · 2020-08-14T15:37:19Z

#36962 introduced an assumption for the upper limit of the enum's value. This PR adds an assumption to the lower value as well.

I've modified the original codegen test to show that derived (in that case, adding 1) values also don't generate bounds checks.

However, this test is actually carefully crafted to not hit a bug: if the enum's variants are modified to 1 and 2 instead of 2 and 3, the test fails by adding a bounds check. I suppose this is an LLVM issue and #75525, while not exactly in this context should be tracking it.

I'm not at all confident if this patch can be accepted, or even if it should be accepted in this state. But I'm curious about what others think :)

~~Improves~~ Should improve #13926 but does not close it because it's not exactly predictable, where bounds checks may pop up against the assumptions.

rust-highfive · 2020-08-14T15:37:23Z

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

bugadani · 2020-08-14T15:58:30Z

Looks like it's even more brittle than I expected. How should I proceed?

src/test/codegen/enum-bounds-check.rs

bugadani · 2020-08-21T18:44:33Z

I found the min-llvm-version option to filter the test. However, should I mark the whole test for llvm >= 10, or split it up into 2 tests and just filter the part added by this PR?

estebank · 2020-08-21T23:40:22Z

@bugadani splitting the test sounds like the best course of action.

bugadani · 2020-08-24T09:37:17Z

Is there anything more I should do?

lcnr · 2020-08-24T11:39:46Z

hmm, this looks fairly innocent to me but I don't know too much about this. Is there anything actionable left for #13926, because otherwise I would prefer to close that issue and open separate issues for the remaining cases. Afaict this does fix #13926 (comment).

I would prefer for someone else to take this over.

Maybe r? @eddyb considering that they reviewed #36962

bugadani · 2020-08-24T12:27:52Z

One problem is that LLVM seems a bit limited in how it handles assumptions and because of this, this patch currently only works for some values, but not for others. This is why I think that issue can't yet be closed, but this is not something that can be fixed this way.

The current test case is a bit of a lie because of this: it just demonstrates that some cases will be optimized, but I think I had to hunt for the numbers a bit since others did not get optimized...

lcnr · 2020-08-24T13:01:27Z

Hmm, can you then add some of these not yet optimized cases as a comment to the issue so it's clear what still needs to be done here?

eddyb · 2020-08-24T15:51:20Z

src/librustc_codegen_ssa/mir/rvalue.rs

+                                    let cmp_end =
+                                        bx.icmp(IntPredicate::IntULE, llval, ll_t_in_const);
+
+                                    if *scalar.valid_range.start() > 0 {


Note to self/other reviewers: this condition should be enough because this is inside an if that checks for scalar.valid_range.end() > scalar.valid_range.start() (i.e. that the range does not wrap around, making it a valid simple unsigned integer range).

We could probably also support wraparound ranges, by treating them as signed integer ranges, but I guess that's less commonly used, especially for indexing?

src/librustc_codegen_ssa/mir/rvalue.rs

eddyb · 2020-08-24T15:52:51Z

I'm worried assume still has downsides, so:
@bors try @rust-timer queue

rust-timer · 2020-08-24T15:52:52Z

Awaiting bors try build completion

bors · 2020-08-24T15:53:05Z

⌛ Trying commit 6e670726116812a156cd0dd14cad1e5c58669f27 with merge 8f6a7e4210495c2a5cc5b0ab704b7ce17f2c5a35...

eddyb

LGTM, but I'll let someone else have the final say wrt codegen

eddyb · 2020-08-24T15:53:21Z

r? @nagisa or @nikic

bors · 2020-08-24T16:37:00Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 8f6a7e4210495c2a5cc5b0ab704b7ce17f2c5a35 (8f6a7e4210495c2a5cc5b0ab704b7ce17f2c5a35)

rust-timer · 2020-08-24T16:37:01Z

Queued 8f6a7e4210495c2a5cc5b0ab704b7ce17f2c5a35 with parent 0301700, future comparison URL.

rust-timer · 2020-08-24T17:53:18Z

Finished benchmarking try commit (8f6a7e4210495c2a5cc5b0ab704b7ce17f2c5a35): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

bugadani · 2020-08-24T18:15:48Z

Hmm, can you then add some of these not yet optimized cases as a comment to the issue so it's clear what still needs to be done here?

~This one fails: ~ Correction: this one passes with llvm11

// compile-flags: -O
// min-llvm-version: 10.0

#![crate_type = "lib"]

pub enum Bar {
    A = 1,
    B = 2
}

// CHECK-LABEL: @lookup_inc
#[no_mangle]
pub fn lookup_inc(buf: &[u8; 5], f: Bar) -> u8 {
    // CHECK-NOT: panic_bounds_check
    buf[f as usize + 1]
}

// CHECK-LABEL: @lookup_dec
#[no_mangle]
pub fn lookup_dec(buf: &[u8; 5], f: Bar) -> u8 {
    // CHECK-NOT: panic_bounds_check
    buf[f as usize - 1]
}

So does the original case:

// compile-flags: -O
// min-llvm-version: 10.0

#![crate_type = "lib"]

#[repr(u8)]
pub enum Exception {
    Low = 5,
    High = 10,
}

// CHECK-LABEL: @access
#[no_mangle]
pub fn access(array: &[usize; 12], exc: Exception) -> usize {
    // CHECK-NOT: panic_bounds_check
    array[(exc as u8 - 4) as usize]
}

In fact, it is harder to find cases where the test passes. The only explanation I can think of is that LLVM has some other idea about the assumptions than me.

nagisa

I don’t mind adding this either, but I wouldn’t hold my breath that any specific behaviour will last, especially across backend versions.

I’m saying this specifically because I don’t think the behaviour added tests verify is something we want to guarantee. It is sure nice when the optimisations just work out and manage to produce exactly the desired outcome, but I wouldn’t block e.g. LLVM upgrade over this test failing. This is also relevant to e.g. users that build with an external LLVM (such as debian).

I guess we can try to remember that these are not actually super important if we find them failing? Perhaps a comment would be good to somehow indicate that these tests are not normative.

otherwise r=me

src/librustc_codegen_ssa/mir/rvalue.rs

nagisa · 2020-08-31T23:11:59Z

Yeah this particular failure has been spuriously recently on many different PRs.

bors · 2020-09-01T00:19:07Z

⌛ Testing commit 1d157ce with merge 37ddb0103c726415513dd29225f339dc04627d4a...

rust-log-analyzer · 2020-09-01T00:24:47Z

The job dist-i686-msvc of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

bors · 2020-09-01T00:25:57Z

💔 Test failed - checks-actions

bugadani · 2020-09-01T06:13:42Z

Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

I love it :) Especially the empty log excerpt, which is actually accurate since the logs indicate network error :)

@nagisa do you get notified about these, or should I keep pinging you?

lcnr · 2020-09-01T06:51:56Z

@bors retry

bors · 2020-09-01T07:26:29Z

⌛ Testing commit 1d157ce with merge b170fe1121dde6e9a7684038c8ccc5cb49f0c257...

bors · 2020-09-01T07:28:55Z

💔 Test failed - checks-actions

rust-log-analyzer · 2020-09-01T07:32:04Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

nagisa · 2020-09-01T12:47:47Z

@bors retry

…

On Tue, 1 Sep 2020, 10:32 Rust Log Analyzer, ***@***.***> wrote: Your PR failed <https://github.com/rust-lang-ci/rust/runs/1054997634> (pretty log <https://rust-lang.github.io/rust-log-analyzer/log-viewer/#https://github.com/rust-lang-ci/rust/commit/b170fe1121dde6e9a7684038c8ccc5cb49f0c257/checks/1054997634/logs>, raw log <https://github.com/rust-lang-ci/rust/commit/b170fe1121dde6e9a7684038c8ccc5cb49f0c257/checks/1054997634/logs>). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. *Click to expand the log.* I'm a bot <https://github.com/rust-ops/rust-log-analyzer>! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact ***@***.***/infra*. (Feature Requests <https://github.com/rust-ops/rust-log-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3Afeature-request> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#75529 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFFZUXGW5RCVGKGOPBPLTLSDSPQJANCNFSM4P7TXCQA> .

bors · 2020-09-01T13:36:58Z

⌛ Testing commit 1d157ce with merge 397db05...

bors · 2020-09-01T15:42:20Z

☀️ Test successful - checks-actions, checks-azure
Approved by: nagisa
Pushing 397db05 to master...

rust-highfive assigned lcnr Aug 14, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2020

bugadani force-pushed the bounds-check branch from 4a37495 to 71df7f6 Compare August 14, 2020 15:37

estebank reviewed Aug 14, 2020

View reviewed changes

src/test/codegen/enum-bounds-check.rs Outdated Show resolved Hide resolved

rust-highfive assigned eddyb and unassigned lcnr Aug 24, 2020

eddyb reviewed Aug 24, 2020

View reviewed changes

src/librustc_codegen_ssa/mir/rvalue.rs Outdated Show resolved Hide resolved

eddyb approved these changes Aug 24, 2020

View reviewed changes

rust-highfive assigned nagisa and unassigned eddyb Aug 24, 2020

nagisa reviewed Aug 25, 2020

View reviewed changes

src/librustc_codegen_ssa/mir/rvalue.rs Outdated Show resolved Hide resolved

bugadani force-pushed the bounds-check branch from 1c899bc to 17a045b Compare August 25, 2020 08:59

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 Aug 31, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 1, 2020

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 Sep 1, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 1, 2020

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 Sep 1, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2020

bors merged commit 397db05 into rust-lang:master Sep 1, 2020

erikdesjardins mentioned this pull request Sep 16, 2020

Passing enum by-value loses range metadata #13926

Closed

bugadani deleted the bounds-check branch October 16, 2020 14:53

eddyb mentioned this pull request May 7, 2022

Fix repr(align) enum handling #96814

Merged

cuviper added this to the 1.48.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate some other bound checks when index comes from an enum #75529

Eliminate some other bound checks when index comes from an enum #75529

bugadani commented Aug 14, 2020 •

edited

Loading

rust-highfive commented Aug 14, 2020

bugadani commented Aug 14, 2020

bugadani commented Aug 21, 2020

estebank commented Aug 21, 2020

bugadani commented Aug 24, 2020

lcnr commented Aug 24, 2020 •

edited

Loading

bugadani commented Aug 24, 2020 •

edited

Loading

lcnr commented Aug 24, 2020 •

edited

Loading

eddyb Aug 24, 2020

eddyb commented Aug 24, 2020

rust-timer commented Aug 24, 2020

bors commented Aug 24, 2020

eddyb left a comment

eddyb commented Aug 24, 2020

bors commented Aug 24, 2020

rust-timer commented Aug 24, 2020

rust-timer commented Aug 24, 2020

bugadani commented Aug 24, 2020 •

edited

Loading

nagisa left a comment

nagisa commented Aug 31, 2020

bors commented Sep 1, 2020

rust-log-analyzer commented Sep 1, 2020

bors commented Sep 1, 2020

bugadani commented Sep 1, 2020

lcnr commented Sep 1, 2020

bors commented Sep 1, 2020

bors commented Sep 1, 2020

rust-log-analyzer commented Sep 1, 2020

nagisa commented Sep 1, 2020 via email

bors commented Sep 1, 2020

bors commented Sep 1, 2020

Eliminate some other bound checks when index comes from an enum #75529

Eliminate some other bound checks when index comes from an enum #75529

Conversation

bugadani commented Aug 14, 2020 • edited Loading

rust-highfive commented Aug 14, 2020

bugadani commented Aug 14, 2020

bugadani commented Aug 21, 2020

estebank commented Aug 21, 2020

bugadani commented Aug 24, 2020

lcnr commented Aug 24, 2020 • edited Loading

bugadani commented Aug 24, 2020 • edited Loading

lcnr commented Aug 24, 2020 • edited Loading

eddyb Aug 24, 2020

Choose a reason for hiding this comment

eddyb commented Aug 24, 2020

rust-timer commented Aug 24, 2020

bors commented Aug 24, 2020

eddyb left a comment

Choose a reason for hiding this comment

eddyb commented Aug 24, 2020

bors commented Aug 24, 2020

rust-timer commented Aug 24, 2020

rust-timer commented Aug 24, 2020

bugadani commented Aug 24, 2020 • edited Loading

nagisa left a comment

Choose a reason for hiding this comment

nagisa commented Aug 31, 2020

bors commented Sep 1, 2020

rust-log-analyzer commented Sep 1, 2020

bors commented Sep 1, 2020

bugadani commented Sep 1, 2020

lcnr commented Sep 1, 2020

bors commented Sep 1, 2020

bors commented Sep 1, 2020

rust-log-analyzer commented Sep 1, 2020

nagisa commented Sep 1, 2020 via email

bors commented Sep 1, 2020

bors commented Sep 1, 2020

bugadani commented Aug 14, 2020 •

edited

Loading

lcnr commented Aug 24, 2020 •

edited

Loading

bugadani commented Aug 24, 2020 •

edited

Loading

lcnr commented Aug 24, 2020 •

edited

Loading

bugadani commented Aug 24, 2020 •

edited

Loading