Skip to content

Commit

Permalink
Don't eliminate variables due to issue #12475
Browse files Browse the repository at this point in the history
  • Loading branch information
@brmataptos
brmataptos committed Mar 12, 2024
1 parent 930faf8 commit c3fbf42
Show file tree
Hide file tree
Showing 14 changed files with 134 additions and 186 deletions.
56 changes: 31 additions & 25 deletions third_party/move/move-compiler-v2/src/ast_simplifier.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -987,33 +987,39 @@ impl<'env> ExpRewriterFunctions for SimplifierRewriter<'env> {
return Some(body.clone());
}

// (3) If some pattern vars are unused in the body, turn them into wildcards.
let new_pat = if !unused_bound_vars.is_empty() {
Some(pat.clone().remove_vars(&unused_bound_vars))
} else {
None
};
// The following is disabled for now until we figure out whether there is a fix
// for #12475. If that is fixed, then we can safely rewrite unused variable
// definitions to wildcards.
//
// // (3) If some pattern vars are unused in the body, turn them into wildcards.
// let new_pat = if !unused_bound_vars.is_empty() {
// Some(pat.clone().remove_vars(&unused_bound_vars))
// } else {
// None
// };

// Ideas not yet implemented:
// (4) simplify the pattern: if subpat is wildcard and subexpr is side-effect-free,
// can remove it and corresponding subexpr.
// (5) simplify the pattern: if subpat is wildcard, corresponding subexpr can be
// simplified to not produce a value
// (6) if body is also a block and its binding has no references to our bound vars,
// then merge patterns and blocks
// (7) if pattern is a singleton `Tuple` and binding is a `Tuple`, turn it into let x = val.
// // Ideas not yet implemented:
// // (4) simplify the pattern: if subpat is wildcard and subexpr is side-effect-free,
// // can remove it and corresponding subexpr.
// // (5) simplify the pattern: if subpat is wildcard, corresponding subexpr can be
// // simplified to not produce a value
// // (6) if body is also a block and its binding has no references to our bound vars,
// // then merge patterns and blocks
// // (7) if pattern is a singleton `Tuple` and binding is a `Tuple`, turn it into let x = val.

if let Some(pat) = new_pat {
let exp = ExpData::Block(id, pat, opt_binding.clone(), body.clone()).into_exp();
trace!(
"Dropping some vars for rewrite_block(id={}), result = {}",
id.as_usize(),
exp.display_verbose(self.env()),
);
Some(exp)
} else {
None
}
// if let Some(pat) = new_pat {
// let exp = ExpData::Block(id, pat, opt_binding.clone(), body.clone()).into_exp();
// trace!(
// "Dropping some vars for rewrite_block(id={}), result = {}",
// id.as_usize(),
// exp.display_verbose(self.env()),
// );
// Some(exp)
// } else {
// None
// }

None
}

fn rewrite_if_else(&mut self, _id: NodeId, cond: &Exp, then: &Exp, else_: &Exp) -> Option<Exp> {
Expand Down
1 change: 1 addition & 0 deletions third_party/move/move-compiler-v2/src/bytecode_generator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,7 @@ impl<'env> Generator<'env> {
match pat {
Pattern::Wildcard(_) => {
// Nothing to do
// TODO(#12475) Should we copy to a temp here?
},
Pattern::Var(var_id, sym) => {
let local = self.find_local_for_pattern(*var_id, *sym, next_scope);
Expand Down
9 changes: 6 additions & 3 deletions third_party/move/move-compiler-v2/tests/checking/inlining/bug_11223.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ module 0xcafe::vectors {
{
let flipsref5: &vector<u8> = Borrow(Immutable)(flips);
{
let _: vector<u8> = Copy(flips);
let _v: vector<u8> = Copy(flips);
{
let x: &vector<u8> = flipsref5;
vector::length<u8>(x)
let _v2: vector<u8> = flips;
{
let x: &vector<u8> = flipsref5;
vector::length<u8>(x)
}
}
}
}
Expand Down
28 changes: 20 additions & 8 deletions third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ module 0xcafe::vectors {
}
};
{
let _: vector<u8> = Copy(flips);
Tuple()
let _v: vector<u8> = Copy(flips);
{
let _v2: vector<u8> = flips;
Tuple()
}
}
}
public entry fun guess_flips_directly(flips: vector<u8>) {
Expand All @@ -44,16 +47,22 @@ module 0xcafe::vectors {
}
};
{
let _: vector<u8> = Copy(flips);
Tuple()
let _v: vector<u8> = Copy(flips);
{
let _v2: vector<u8> = flips;
Tuple()
}
}
}
}
public entry fun guess_with_break_without_inline(flips: vector<u8>) {
vectors::loops_with_break_no_inline(Borrow(Immutable)(flips));
{
let _: vector<u8> = Copy(flips);
Tuple()
let _v: vector<u8> = Copy(flips);
{
let _v2: vector<u8> = flips;
Tuple()
}
}
}
public entry fun guess_without_break_with_inline(flips: vector<u8>) {
Expand All @@ -78,8 +87,11 @@ module 0xcafe::vectors {
}
};
{
let _: vector<u8> = Copy(flips);
Tuple()
let _v: vector<u8> = flips;
{
let _v2: vector<u8> = Copy(flips);
Tuple()
}
}
}
private fun loops_with_break_no_inline(flips: &vector<u8>) {
Expand Down
9 changes: 6 additions & 3 deletions third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717_looponly.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,13 @@ module 0xcafe::vectors {
}
};
{
let _: vector<u8> = Copy(flips);
let _v: vector<u8> = Copy(flips);
{
let x: &vector<u8> = flipsref5;
vector::length<u8>(x)
let _v2: vector<u8> = flips;
{
let x: &vector<u8> = flipsref5;
vector::length<u8>(x)
}
}
}
}
Expand Down
86 changes: 12 additions & 74 deletions third_party/move/move-compiler-v2/tests/simplifier-elimination/assign_unpack_references.exp
View file
Original file line number Diff line number Diff line change
@@ -1,77 +1,3 @@

Diagnostics:
warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:8:65
│
8 │ R { s1: S { f }, s2 } = R { s1: S{f: 0}, s2: S{f: 1} }; f; s2;
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:8:68
│
8 │ R { s1: S { f }, s2 } = R { s1: S{f: 0}, s2: S{f: 1} }; f; s2;
│ ^^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:11:9
│
11 │ f; s2;
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:11:12
│
11 │ f; s2;
│ ^^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:17:66
│
17 │ R { s1: S { f }, s2 } = &R { s1: S{f: 0}, s2: S{f: 1} }; f; s2;
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:17:69
│
17 │ R { s1: S { f }, s2 } = &R { s1: S{f: 0}, s2: S{f: 1} }; f; s2;
│ ^^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:20:9
│
20 │ f; s2;
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:20:12
│
20 │ f; s2;
│ ^^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:27:70
│
27 │ R { s1: S { f }, s2 } = &mut R { s1: S{f: 0}, s2: S{f: 1} }; f; s2;
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:27:73
│
27 │ R { s1: S { f }, s2 } = &mut R { s1: S{f: 0}, s2: S{f: 1} }; f; s2;
│ ^^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:30:9
│
30 │ f; s2;
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/assign_unpack_references.move:30:12
│
30 │ f; s2;
│ ^^

// -- Model dump before bytecode pipeline
module 0x8675309::M {
struct R {
Expand All @@ -87,8 +13,12 @@ module 0x8675309::M {
{
let s2: M::S;
M::R{ s1: M::S{ f: f: u64 }, s2: s2: M::S }: M::R = pack M::R(pack M::S(0), pack M::S(1));
f;
s2;
f: u64 = 0;
s2: M::S = pack M::S(0);
f;
s2;
Tuple()
}
}
Expand All @@ -99,8 +29,12 @@ module 0x8675309::M {
{
let s2: &M::S;
M::R{ s1: M::S{ f: f: &u64 }, s2: s2: &M::S }: M::R = Borrow(Immutable)(pack M::R(pack M::S(0), pack M::S(1)));
f;
s2;
f: &u64 = Borrow(Immutable)(0);
s2: &M::S = Borrow(Immutable)(pack M::S(0));
f;
s2;
Tuple()
}
}
Expand All @@ -111,8 +45,12 @@ module 0x8675309::M {
{
let s2: &mut M::S;
M::R{ s1: M::S{ f: f: &mut u64 }, s2: s2: &mut M::S }: M::R = Borrow(Mutable)(pack M::R(pack M::S(0), pack M::S(1)));
f;
s2;
f: &mut u64 = Borrow(Mutable)(0);
s2: &mut M::S = Borrow(Mutable)(pack M::S(0));
f;
s2;
Tuple()
}
}
Expand Down
2 changes: 1 addition & 1 deletion third_party/move/move-compiler-v2/tests/simplifier-elimination/bind_with_type_annot.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ module 0x8675309::M {
}
private fun t0() {
{
let (_, _, M::R{ f: _ }): (u64, bool, M::R) = Tuple(0, false, pack M::R(0));
let (x: u64, b: bool, M::R{ f: f: u64 }): (u64, bool, M::R) = Tuple(0, false, pack M::R(0));
Tuple()
}
}
Expand Down
6 changes: 0 additions & 6 deletions third_party/move/move-compiler-v2/tests/simplifier-elimination/else_assigns_if_doesnt.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ warning: If condition is always true, so else branch code eliminated as dead cod
10 │ │ };
│ ╰─────' else branch eliminated

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/else_assigns_if_doesnt.move:9:9
│
9 │ x;
│ ^

// -- Model dump before bytecode pipeline
module <SELF>_0 {
private fun main() {
Expand Down
6 changes: 0 additions & 6 deletions third_party/move/move-compiler-v2/tests/simplifier-elimination/if_assigns_else_doesnt.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ warning: If condition is always true, so else branch code eliminated as dead cod
10 │ │ };
│ ╰─────' else branch eliminated

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/if_assigns_else_doesnt.move:9:9
│
9 │ y;
│ ^

// -- Model dump before bytecode pipeline
module <SELF>_0 {
private fun main() {
Expand Down
15 changes: 14 additions & 1 deletion third_party/move/move-compiler-v2/tests/simplifier-elimination/moved_var_not_simplified2.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ module 0xc0ffee::m {
let x: u8 = 40;
{
let y: u8 = Move(x);
y
{
let _z: u8 = x;
y
}
}
}
}
Expand All @@ -22,3 +25,13 @@ module 0xc0ffee::m {
}
}
} // end 0xc0ffee::m


Diagnostics:
error: cannot move local `x` since it is still in use
┌─ tests/simplifier-elimination/moved_var_not_simplified2.move:4:17
│
4 │ let y = move x;
│ ^^^^^^ attempted to move here
5 │ let _z = x;
│ -- used here
27 changes: 5 additions & 22 deletions third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_loop.exp
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,3 @@

Diagnostics:
warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/use_before_assign_loop.move:4:43
│
4 │ loop { let y = move x + 1; x = 0; y; }
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/use_before_assign_loop.move:9:62
│
9 │ loop { let y = x + 1; if (cond) { continue }; x = 0; y; }
│ ^

warning: Expression value unused and side-effect free, so eliminated as dead code
┌─ tests/simplifier-elimination/use_before_assign_loop.move:20:9
│
20 │ x;
│ ^

// -- Model dump before bytecode pipeline
module 0x8675309::M {
private fun tborrow1() {
Expand Down Expand Up @@ -47,6 +27,7 @@ module 0x8675309::M {
break
}
};
x;
Tuple()
}
}
Expand All @@ -55,13 +36,14 @@ module 0x8675309::M {
let x: u64;
loop {
{
let _: u64 = Add<u64>(x, 1);
let y: u64 = Add<u64>(x, 1);
if cond {
continue
} else {
Tuple()
};
x: u64 = 0;
y;
Tuple()
}
}
Expand All @@ -72,8 +54,9 @@ module 0x8675309::M {
let x: u64;
loop {
{
let _: u64 = Add<u64>(Move(x), 1);
let y: u64 = Add<u64>(Move(x), 1);
x: u64 = 0;
y;
Tuple()
}
}
Expand Down
Loading

0 comments on commit c3fbf42

Please sign in to comment.