Array `.len()` MIR optimization pass #86525

shamatar · 2021-06-21T22:16:24Z

This pass kind-of works back the [T; N].len() call that at the moment is first coerced as &[T; N] -> &[T] and then uses &[T].len(). Depends on #86383

rust-highfive · 2021-06-21T22:16:26Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LeSeulArtichaut (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

shamatar · 2021-06-21T22:17:29Z

Due to dependency it's better to look at compiler/rustc_mir/src/transform/normalize_array_len.rs only for now.

The main pain point is that the only way to deal with an expression like this _6 = move _7 as &[u8] (Pointer(Unsize)); in a sound way is either more complex analysis in attempt to patch into _6 = move _7; without unsize cast, or take an approach of extra copy (as done in this PR) assuming that after _6 = move _7 as &[u8] (Pointer(Unsize)); some expression can use _6 being slice or even reassign into it

LeSeulArtichaut · 2021-06-21T22:26:15Z

r? @oli-obk

shamatar · 2021-06-21T22:32:52Z

There is an alternative approach to run this pass somewhat later, and assume that after previous passes we are only interested in cases when the destination of cast is only touched in the Len and then dead by the end of the block without any other modifications

          _6 = move _7 as &[u8] (Pointer(Unsize));
          StorageDead(_7);
          _5 = Len((*_6)); 
          StorageDead(_6);

Even more alternative approach is to go full scale dataflow analysis and be able to remove a unsize cast by proving that all other potentially dependent statements are ok with _6 being &[T; N]

shamatar · 2021-06-21T22:49:02Z

@bors try

All tests passed locally, problem was due to not committed files

bors · 2021-06-21T22:49:03Z

@shamatar: 🔑 Insufficient privileges: not in try users

shamatar · 2021-06-21T22:57:35Z

I'll add for completeness

@rustbot label: +S-waiting-on-review

klensy · 2021-06-21T23:03:57Z

Isn't this https://github.com/rust-lang/rust/blob/406d4a9cc3b9601cf98a07c6c83e0227d64f5d48/compiler/rustc_mir/src/transform/instcombine.rs#L116..L131 ?

shamatar · 2021-06-21T23:06:11Z

For it to work an operand of Len has to be [T; N], but without this pass it's &[T] in usual situations here. You can see it in the src/test/mir-opt/issue_76432.test.SimplifyComparisonIntegral.diff changes that are a by-product, Len() that was there before was optimized down to constant

klensy · 2021-06-21T23:15:31Z

src/test/mir-opt/const_prop/slice_len.main.ConstProp.64bit.diff

 -         _8 = Lt(_6, _7);                 // scope 0 at $DIR/slice_len.rs:5:5: 5:33
 -         assert(move _8, "index out of bounds: the length is {} but the index is {}", move _7, _6) -> bb1; // scope 0 at $DIR/slice_len.rs:5:5: 5:33
-+         _7 = const 3_usize;              // scope 0 at $DIR/slice_len.rs:5:5: 5:33
 +         _8 = const true;                 // scope 0 at $DIR/slice_len.rs:5:5: 5:33
 +         assert(const true, "index out of bounds: the length is {} but the index is {}", const 3_usize, const 1_usize) -> bb1; // scope 0 at $DIR/slice_len.rs:5:5: 5:33


Hmm, shouldn't be this assert removed, as it const true? Or maybe in other pass, i don't really understand this thing deep enough.

It may be removed, but the test doesn't output the results of other passes, just the diff due to application of ConstProp one

We run a SimplifyBranches pass right after ConstProp which will turn the assert into goto -> bb1

wesleywiser · 2021-06-22T18:11:58Z

@bors try @rust-timer queue

rust-timer · 2021-06-22T18:11:59Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-06-22T18:12:08Z

⌛ Trying commit 87797843fb2598134ec8ecc86dc718c0d4657a22 with merge d903a20d696dba2fa37a25fe955133069819febc...

shamatar · 2021-06-22T18:41:36Z

@rustbot label: -S-blocked

bors · 2021-06-22T18:44:02Z

💔 Test failed - checks-actions

shamatar · 2021-10-06T17:50:26Z

@JohnCSimon Done

compiler/rustc_mir_transform/src/normalize_array_len.rs

oli-obk · 2021-10-06T20:20:13Z

@bors r+

Thanks for bearing with us while we figure out our process for reviewing mir opts with the little review capacity we have in wg-mir-opt.

bors · 2021-10-06T20:20:14Z

📌 Commit a31518f has been approved by oli-obk

bors · 2021-10-06T23:12:49Z

⌛ Testing commit a31518f with merge 5c402bc869d4e1188ec5974820481d49b1040322...

bors · 2021-10-06T23:18:38Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-10-06T23:19:06Z

The job i686-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

shamatar · 2021-10-07T06:22:35Z

Looks like random error during CI execution

oli-obk · 2021-10-07T06:39:06Z

@bors retry

bors · 2021-10-07T11:34:43Z

⌛ Testing commit a31518f with merge 680ff86...

bors · 2021-10-07T14:22:10Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 680ff86 to master...

bors · 2021-10-07T14:22:16Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 680ff86 to master...

rust-timer · 2021-10-07T16:26:38Z

Finished benchmarking commit (680ff86): 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

rust-highfive assigned LeSeulArtichaut Jun 21, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2021

LeSeulArtichaut added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2021

rust-highfive assigned oli-obk and unassigned LeSeulArtichaut Jun 21, 2021

This comment has been minimized.

Sign in to view

shamatar mentioned this pull request Jun 21, 2021

Add separate array length function #86404

Closed

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2021

klensy reviewed Jun 21, 2021

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 22, 2021

rustbot removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 22, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 22, 2021

tidy

8562d94

This comment has been minimized.

Sign in to view

run remaining tests

4c3465c

fee1-dead added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2021

oli-obk reviewed Oct 6, 2021

View reviewed changes

compiler/rustc_mir_transform/src/normalize_array_len.rs Outdated Show resolved Hide resolved

opt-level >= 4

a31518f

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 6, 2021

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 Oct 6, 2021

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 7, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 7, 2021

bors merged commit 680ff86 into rust-lang:master Oct 7, 2021

rustbot added this to the 1.57.0 milestone Oct 7, 2021

shamatar deleted the array_len_opt branch February 29, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array `.len()` MIR optimization pass #86525

Array `.len()` MIR optimization pass #86525

shamatar commented Jun 21, 2021

rust-highfive commented Jun 21, 2021

shamatar commented Jun 21, 2021 •

edited

Loading

LeSeulArtichaut commented Jun 21, 2021

This comment has been minimized.

shamatar commented Jun 21, 2021 •

edited

Loading

shamatar commented Jun 21, 2021 •

edited

Loading

bors commented Jun 21, 2021

shamatar commented Jun 21, 2021

klensy commented Jun 21, 2021

shamatar commented Jun 21, 2021 •

edited

Loading

klensy Jun 21, 2021 •

edited

Loading

shamatar Jun 21, 2021

wesleywiser Jun 22, 2021

wesleywiser commented Jun 22, 2021

rust-timer commented Jun 22, 2021

bors commented Jun 22, 2021

shamatar commented Jun 22, 2021

bors commented Jun 22, 2021

This comment has been minimized.

This comment has been minimized.

shamatar commented Oct 6, 2021

oli-obk commented Oct 6, 2021

bors commented Oct 6, 2021

bors commented Oct 6, 2021

bors commented Oct 6, 2021

rust-log-analyzer commented Oct 6, 2021

shamatar commented Oct 7, 2021

oli-obk commented Oct 7, 2021

bors commented Oct 7, 2021

bors commented Oct 7, 2021

bors commented Oct 7, 2021

rust-timer commented Oct 7, 2021

Array .len() MIR optimization pass #86525

Array .len() MIR optimization pass #86525

Conversation

shamatar commented Jun 21, 2021

rust-highfive commented Jun 21, 2021

shamatar commented Jun 21, 2021 • edited Loading

LeSeulArtichaut commented Jun 21, 2021

This comment has been minimized.

shamatar commented Jun 21, 2021 • edited Loading

shamatar commented Jun 21, 2021 • edited Loading

bors commented Jun 21, 2021

shamatar commented Jun 21, 2021

klensy commented Jun 21, 2021

shamatar commented Jun 21, 2021 • edited Loading

klensy Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

shamatar Jun 21, 2021

Choose a reason for hiding this comment

wesleywiser Jun 22, 2021

Choose a reason for hiding this comment

wesleywiser commented Jun 22, 2021

rust-timer commented Jun 22, 2021

bors commented Jun 22, 2021

shamatar commented Jun 22, 2021

bors commented Jun 22, 2021

This comment has been minimized.

This comment has been minimized.

shamatar commented Oct 6, 2021

oli-obk commented Oct 6, 2021

bors commented Oct 6, 2021

bors commented Oct 6, 2021

bors commented Oct 6, 2021

rust-log-analyzer commented Oct 6, 2021

shamatar commented Oct 7, 2021

oli-obk commented Oct 7, 2021

bors commented Oct 7, 2021

bors commented Oct 7, 2021

bors commented Oct 7, 2021

rust-timer commented Oct 7, 2021

Array `.len()` MIR optimization pass #86525

Array `.len()` MIR optimization pass #86525

shamatar commented Jun 21, 2021 •

edited

Loading

shamatar commented Jun 21, 2021 •

edited

Loading

shamatar commented Jun 21, 2021 •

edited

Loading

shamatar commented Jun 21, 2021 •

edited

Loading

klensy Jun 21, 2021 •

edited

Loading