rustc_mir_transform: Add a local value numbering pass, off by default. #92051

pcwalton · 2021-12-17T23:04:16Z

This commit adds a local value numbering pass, which is intended to
eventually subsume constant and copy propagation. It's designed to detect
whether a value has been computed multiple times and replace such duplicate
computations with a reference to the originally-computed value. LLVM performs
this optimization, but it's limited because it doesn't have the ability to
reason about immutability of memory the way Rust MIR can. For example, this
pass optimizes the following code:

let s1: &S = ...;
let s2: &mut S = ...;
let mut sum = 0;
sum += s1.a + s1.b;
s2.a = 1;
sum += s1.a + s1.b;

into:

let s1: &S = ...;
let s2: &mut S = ...;
let mut sum = 0;
sum += s1.a + s1.b;
s2.a = 1;
sum += sum;

LLVM won't do this optimization because it can't prove that the assignment to a
field of s2 doesn't mutate the fields of s1.

Because this pass is likely to have bugs and may be slow, I've turned it off by
default for now. I didn't notice any show-stopping bugs in the test suite, but
out of an abundance of caution I'm keeping it off.

Closes #91688.

rust-highfive · 2021-12-17T23:04:19Z

r? @davidtwco

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

camelid · 2021-12-17T23:31:20Z

Cargo.lock

@@ -2451,9 +2451,9 @@ checksum = "dd20eec3dbe4376829cb7d80ae6ac45e0a766831dca50202ff2d40db46a8a024"

 [[package]]
 name = "os_info"
-version = "3.0.7"
+version = "3.0.8"


Is this intentionally being updated?

ecstatic-morse · 2021-12-17T23:46:47Z

compiler/rustc_mir_transform/src/value_numbering.rs

+        let terminator = match block.terminator {
+            None => return,
+            Some(ref mut terminator) => terminator,
+        };


Once the MIR is built, Terminator is guaranteed to be Some (unless you've moved from it elsewhere in this pass). You're ~~free~~ strongly encouraged to call the terminator and terminator_mut methods.

compiler/rustc_mir_transform/src/value_numbering.rs

ecstatic-morse · 2021-12-18T00:15:42Z

compiler/rustc_mir_transform/src/value_numbering.rs

+        // First, promote mutable locals to immutable ones. This helps our rudimentary alias
+        // analysis a lot.


The precedent for indirect mutation is to traverse the MIR to looking for any locals that are mutably borrowed at any point. I'm not sure if you need to look at the mutability of a LocalDecl separately, but maybe doing it that way helps you avoid dealing with that failing test case?

I'm not sure what you mean, could you elaborate?

Sure, see this comment for context. My understanding is that you're using the mutability field of each LocalDecl as a conservative approximation for whether it can be mutated indirectly (through a pointer). Is that correct? What you're doing is fine of course, but it would be slightly more precise to traverse the MIR looking for any local that is the base of an Rvalue::Ref(Mut) or Rvalue::AddrOf. I don't know how much the extra precision will matter.

Well, actually is what you're doing fine? Is the current version correct in the presence of locals with interior mutability?

Look at the code to handle UnsafeCell.

Most of this concern is resolved by the fact that we don't have to care about function calls (duh).

One more question: could you explain what would happen in this (incredibly niche) case?

//! I am in libcore/src/cell.rs, so I have access to `UnsafeCell`'s field. let x = UnsafeCell { value: 1 }; // `x` is not marked as a mutable local (I think)? let px = &x; unsafe { *(px as *mut _).value = 2 }; read(x.value);

I think you'll handle this correctly due to several conservative assumptions you've made. UnsafeCell is not Copy, for one, and also you bail out on field projections. Is this description accurate or is something more robust stopping these kinds of shenanigans?

Frankly, I suspect there are other MIR transformations that are unsound if direct accesses to the field of an UnsafeCell are allowed, but it doesn't hurt to ask I guess.

tmiasko · 2021-12-18T10:21:34Z

compiler/rustc_mir_transform/src/value_numbering.rs

+                ProjectionElem::Deref => match ty_kind {
+                    ty::Ref(_, inner_ty, Mutability::Not) => {
+                        ty_kind = inner_ty.kind();
+                    }
+                    _ => return false,
+                },


Place behind a shared reference might be mutable?

fn f(x: &mut u32) -> u32 { let a = &*x; let c = *a; *x = 2; c } fn main() { let mut x = 1; assert_eq!(f(&mut x), 1); }

$ rustc +stage1 -O b.rs && ./b $ rustc +stage1 -O -Znumber_values b.rs && ./b thread 'main' panicked at 'assertion failed: `(left == right)` left: `2`, right: `1`', b.rs:10:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is because of NLL I guess. I suppose we should check to see whether the live ranges of the immutable reference overlap the mutation site. If they don't we have to kill the immutable reference.

OK, new plan: contents of an immutable reference are considered immutable if and only if the immutable reference either came from a function parameter or has a use reachable from the current basic block. In your example, a has no uses reachable from the BB, so it would be considered potentially mutable. I think this is sound. It would probably be possible to intertwine this pass with NLL data structures to get precise here, but I'm very hesitant to do that because right now MIR optimizations don't interact with NLL borrow checking.

In other words, the reference must not only be immutable, it must be immutably borrowed across the entire basic block.

Here's a modified form of the example that fails to borrow check because it fails the "uses reachable from the statement" check: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2dcd401a1b807854207c732e2153fc3a

And here's an example that shows that the "function parameter" check is sound because the borrow check rejects an attempt to violate it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2dcd401a1b807854207c732e2153fc3a

Note that the optimized example fails the borrow check: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f1a5acf2c7389f89150d9e25f6d1002c

davidtwco · 2022-01-05T16:49:03Z

r? @ecstatic-morse

nnethercote · 2022-02-04T05:57:12Z

What's the status of this? Looks like it's waiting for a review?

ecstatic-morse · 2022-02-04T19:35:15Z

I'm also excited about this PR. However, it's incorrect in at least one case.

Also, I think the way its detecting indirect mutation is misguided. Local value numbering is perfectly able to handle direct reassignment of variables, but AFAICT this PR refuses to optimize code involving variables that are declared with mut. I think the author is using that as a proxy to detect whether a variable can be mutated through a pointer/reference (I asked for clarification).

Looking for mut declarations is fine (although imprecise) for things without interior mutability. For now, it works even for things with interior mutability because the only statement (not terminator) that can write through a shared reference is a StatementKind::Assign to the field of an UnsafeCell, and that's handled explicitly. Everything else goes through a function call. This assumption no longer holds if this pass were extended to global value numbering (as the module docs suggest), since now you have to handle the effects of terminators (function calls, etc.). There's no mention of this anywhere.

I was going to suggest that the pass to look explicitly for mutable references to locals or shared references to locals whose type has interior mutability (!Freeze). However, I think this might be unsound in the global case since Freeze is shallow?

Dylan-DPC · 2022-03-07T07:37:06Z

@pcwalton any updates on this?

bors · 2022-03-18T05:46:04Z

☔ The latest upstream changes (presumably #88098) made this pull request unmergeable. Please resolve the merge conflicts.

pcwalton · 2022-04-09T00:16:10Z

I think the author is using that as a proxy to detect whether a variable can be mutated through a pointer/reference

Yes, that is the goal.

I was going to suggest that the pass to look explicitly for mutable references to locals or shared references to locals whose type has interior mutability (!Freeze).

I would prefer to keep this patch small and conservative for now. Adding more and more precision to it is something that can be done over time. The perfect shouldn't be the enemy of the good, especially when we don't even know what sound semantics of the perfect should be!

pcwalton · 2022-04-13T22:40:13Z

compiler/rustc_mir_transform/src/value_numbering.rs

+                    local_info.statement_index_of_last_kill = location.statement_index;
+                    local_info.current_live_range_index += 1;
+                }
+            }


Note to self: this is where we need to kill immutable references in some cases.

This commit adds a local [value numbering] pass, which is intended to eventually subsume constant and copy propagation. It's designed to detect whether a value has been computed multiple times and replace such duplicate computations with a reference to the originally-computed value. LLVM performs this optimization, but it's limited because it doesn't have the ability to reason about immutability of memory the way Rust MIR can. For example, this pass optimizes [the following code]: let s1: &S = ...; let s2: &mut S = ...; let mut sum = 0; sum += s1.a + s1.b; s2.a = 1; sum += s1.a + s1.b; into: let s1: &S = ...; let s2: &mut S = ...; let mut sum = 0; sum += s1.a + s1.b; s2.a = 1; sum += sum; LLVM won't do this optimization because it can't prove that the assignment to a field of `s2` doesn't mutate the fields of `s1`. Because this pass is likely to have bugs and may be slow, I've turned it off by default for now. I didn't notice any show-stopping bugs in the test suite, but out of an abundance of caution I'm keeping it off. Closes rust-lang#91688. [value numbering]: https://en.wikipedia.org/wiki/Value_numbering [the following code]: https://rust.godbolt.org/z/7f6Gff7Ms

rust-log-analyzer · 2022-04-13T23:31:17Z

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

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

failures:

---- [ui] src/test/ui/destructuring-assignment/slice_destructure.rs stdout ----
normalized stderr:
warning: variable `a` is assigned to, but never used
  --> $DIR/slice_destructure.rs:4:12
   |
LL |   let (mut a, mut b);
   |
   = note: `#[warn(unused_variables)]` on by default
   = note: `#[warn(unused_variables)]` on by default
   = note: consider using `_a` instead

warning: variable `b` is assigned to, but never used
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
  --> $DIR/slice_destructure.rs:4:19
   |
LL |   let (mut a, mut b);
   |
   |
   = note: consider using `_b` instead
warning: value assigned to `a` is never read
  --> $DIR/slice_destructure.rs:5:4
   |
   |
LL |   [a, b] = [0, 1];
   |
   = note: `#[warn(unused_assignments)]` on by default
   = note: `#[warn(unused_assignments)]` on by default
   = help: maybe it is overwritten before being read?

warning: value assigned to `b` is never read
  --> $DIR/slice_destructure.rs:5:7
   |
LL |   [a, b] = [0, 1];
   |
   |
   = help: maybe it is overwritten before being read?
warning: variable does not need to be mutable
  --> $DIR/slice_destructure.rs:4:8
   |
   |
LL |   let (mut a, mut b);
   |        |
   |        help: remove this `mut`
   |
   = note: `#[warn(unused_mut)]` on by default
   = note: `#[warn(unused_mut)]` on by default

warning: variable does not need to be mutable
  --> $DIR/slice_destructure.rs:4:15
   |
LL |   let (mut a, mut b);
   |               |
   |               help: remove this `mut`

warning: variable does not need to be mutable
---
To only update this specific test, also pass `--test-args destructuring-assignment/slice_destructure.rs`

error: 1 errors occurred comparing output.
status: exit status: 0
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/destructuring-assignment/slice_destructure.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/destructuring-assignment/slice_destructure/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/destructuring-assignment/slice_destructure/auxiliary"
stdout: none
--- stderr -------------------------------
warning: variable `a` is assigned to, but never used
   |
   |
LL |   let (mut a, mut b);
   |
   = note: `#[warn(unused_variables)]` on by default
   = note: `#[warn(unused_variables)]` on by default
   = note: consider using `_a` instead

warning: variable `b` is assigned to, but never used
   |
   |
LL |   let (mut a, mut b);
   |
   |
   = note: consider using `_b` instead
warning: value assigned to `a` is never read
  --> /checkout/src/test/ui/destructuring-assignment/slice_destructure.rs:5:4
   |
   |
LL |   [a, b] = [0, 1];
   |
   = note: `#[warn(unused_assignments)]` on by default
   = note: `#[warn(unused_assignments)]` on by default
   = help: maybe it is overwritten before being read?

warning: value assigned to `b` is never read
   |
   |
LL |   [a, b] = [0, 1];
   |
   |
   = help: maybe it is overwritten before being read?
warning: variable does not need to be mutable
  --> /checkout/src/test/ui/destructuring-assignment/slice_destructure.rs:4:8
   |
   |
LL |   let (mut a, mut b);
   |        |
   |        help: remove this `mut`
   |
   = note: `#[warn(unused_mut)]` on by default
   = note: `#[warn(unused_mut)]` on by default

warning: variable does not need to be mutable
  --> /checkout/src/test/ui/destructuring-assignment/slice_destructure.rs:4:15
   |
LL |   let (mut a, mut b);
   |               |
   |               help: remove this `mut`

warning: variable does not need to be mutable

bors · 2022-05-28T14:41:51Z

☔ The latest upstream changes (presumably #97158) made this pull request unmergeable. Please resolve the merge conflicts.

pcwalton · 2022-10-18T23:59:30Z

I'm closing this as it doesn't make sense to work on it until after updated const prop/dest prop land.

rust-highfive assigned davidtwco Dec 17, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 17, 2021

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

This comment has been minimized.

Sign in to view

camelid added the A-mir-opt Area: MIR optimizations label Dec 17, 2021

camelid reviewed Dec 17, 2021

View reviewed changes

ecstatic-morse reviewed Dec 17, 2021

View reviewed changes

compiler/rustc_mir_transform/src/value_numbering.rs Show resolved Hide resolved

ecstatic-morse reviewed Dec 18, 2021

View reviewed changes

compiler/rustc_mir_transform/src/value_numbering.rs Show resolved Hide resolved

ecstatic-morse reviewed Dec 18, 2021

View reviewed changes

tmiasko reviewed Dec 18, 2021

View reviewed changes

rust-highfive assigned ecstatic-morse and unassigned davidtwco Jan 5, 2022

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2022

ecstatic-morse 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 Feb 4, 2022

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 6, 2022

pcwalton commented Apr 13, 2022

View reviewed changes

pcwalton force-pushed the value-numbering branch from 437cafc to e38c31c Compare April 13, 2022 23:10

pcwalton closed this Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_mir_transform: Add a local value numbering pass, off by default. #92051

rustc_mir_transform: Add a local value numbering pass, off by default. #92051

pcwalton commented Dec 17, 2021

rust-highfive commented Dec 17, 2021

This comment has been minimized.

camelid Dec 17, 2021

ecstatic-morse Dec 17, 2021 •

edited

Loading

ecstatic-morse Dec 18, 2021

pcwalton Dec 18, 2021

ecstatic-morse Dec 18, 2021 •

edited

Loading

ecstatic-morse Dec 18, 2021 •

edited

Loading

pcwalton Dec 18, 2021

ecstatic-morse Dec 18, 2021 •

edited

Loading

tmiasko Dec 18, 2021 •

edited

Loading

pcwalton Dec 18, 2021

pcwalton Apr 15, 2022 •

edited

Loading

pcwalton Apr 15, 2022

pcwalton Apr 15, 2022

davidtwco commented Jan 5, 2022

nnethercote commented Feb 4, 2022

ecstatic-morse commented Feb 4, 2022 •

edited

Loading

Dylan-DPC commented Mar 7, 2022

bors commented Mar 18, 2022

pcwalton commented Apr 9, 2022 •

edited

Loading

pcwalton Apr 13, 2022

rust-log-analyzer commented Apr 13, 2022

bors commented May 28, 2022

pcwalton commented Oct 18, 2022

		// First, promote mutable locals to immutable ones. This helps our rudimentary alias
		// analysis a lot.

rustc_mir_transform: Add a local value numbering pass, off by default. #92051

rustc_mir_transform: Add a local value numbering pass, off by default. #92051

Conversation

pcwalton commented Dec 17, 2021

rust-highfive commented Dec 17, 2021

This comment has been minimized.

Choose a reason for hiding this comment

ecstatic-morse Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

tmiasko Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcwalton Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtwco commented Jan 5, 2022

nnethercote commented Feb 4, 2022

ecstatic-morse commented Feb 4, 2022 • edited Loading

Dylan-DPC commented Mar 7, 2022

bors commented Mar 18, 2022

pcwalton commented Apr 9, 2022 • edited Loading

Choose a reason for hiding this comment

rust-log-analyzer commented Apr 13, 2022

bors commented May 28, 2022

pcwalton commented Oct 18, 2022

ecstatic-morse Dec 17, 2021 •

edited

Loading

ecstatic-morse Dec 18, 2021 •

edited

Loading

ecstatic-morse Dec 18, 2021 •

edited

Loading

ecstatic-morse Dec 18, 2021 •

edited

Loading

tmiasko Dec 18, 2021 •

edited

Loading

pcwalton Apr 15, 2022 •

edited

Loading

ecstatic-morse commented Feb 4, 2022 •

edited

Loading

pcwalton commented Apr 9, 2022 •

edited

Loading