Fix a bug in the recent memory copy propagation (FuelLabs#4628)

## Description The bug was discovered when trying to reproduce FuelLabs#4511. During the optimization, we may need to insert new GEPs. Those were being inserted at the start of the block. That led to some use-before-def situations. With this change, we now insert it just before the instruction being optimized. ## Checklist - [ ] I have linked to any relevant issues. - [ ] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [ ] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Joshua Batty <[email protected]>
dsafewgfa · Jun 6, 2023 · 1070c60 · 1070c60
1 parent 7470126
commit 1070c60
Show file tree

Hide file tree

Showing 8 changed files with 163 additions and 27 deletions.
diff --git a/sway-ir/src/optimize/memcpyopt.rs b/sway-ir/src/optimize/memcpyopt.rs
@@ -618,11 +618,19 @@ fn local_copy_prop(
                 }
             }
 
+            if replacements.is_empty() {
+                break;
+            } else {
+                modified = true;
+            }
+
             // If we have any NewGep replacements, insert those new GEPs into the block.
+            // Since the new instructions need to be just before the value load that they're
+            // going to be used in, we copy all the instructions into a new vec
+            // and just replace the contents of the basic block.
             let mut new_insts = vec![];
-            let replacements = replacements
-                .into_iter()
-                .map(|(load_inst, replacement)| {
+            for inst in block.instruction_iter(context) {
+                if let Some(replacement) = replacements.remove(&inst) {
                     let replacement = match replacement {
                         Replacement::OldGep(v) => v,
                         Replacement::NewGep(ReplGep {
@@ -655,31 +663,24 @@ fn local_copy_prop(
                             v
                         }
                     };
-                    (load_inst, replacement)
-                })
-                .collect::<Vec<_>>();
-
-            block.prepend_instructions(context, new_insts);
-
-            for replacement in &replacements {
-                match replacement.0.get_instruction_mut(context) {
-                    Some(Instruction::Load(ref mut src_val_ptr))
-                    | Some(Instruction::MemCopyBytes {
-                        ref mut src_val_ptr,
-                        ..
-                    })
-                    | Some(Instruction::MemCopyVal {
-                        ref mut src_val_ptr,
-                        ..
-                    }) => *src_val_ptr = replacement.1,
-                    _ => panic!("Unexpected instruction type"),
+                    match inst.get_instruction_mut(context) {
+                        Some(Instruction::Load(ref mut src_val_ptr))
+                        | Some(Instruction::MemCopyBytes {
+                            ref mut src_val_ptr,
+                            ..
+                        })
+                        | Some(Instruction::MemCopyVal {
+                            ref mut src_val_ptr,
+                            ..
+                        }) => *src_val_ptr = replacement,
+                        _ => panic!("Unexpected instruction type"),
+                    }
                 }
+                new_insts.push(inst);
             }
-            if !replacements.is_empty() {
-                modified = true;
-            } else {
-                break;
-            }
+
+            // Replace the basic block contents with what we just built.
+            let _ = std::mem::replace(&mut (context.blocks[block.0].instructions), new_insts);
         }
     }
 

diff --git a/sway-ir/tests/dce/copy_prop_1.ir b/sway-ir/tests/dce/copy_prop_1.ir
@@ -0,0 +1,21 @@
+script {
+    entry fn main() -> u64 {
+        local { u64, u64, u64, u64 } __anon_0
+        local { u64, u64, u64, u64 } __anon_468
+
+        entry():
+        v0 = get_local ptr { u64, u64, u64, u64 }, __anon_0
+        v1 = const u64 0
+        v2 = get_elem_ptr v0, ptr u64, v1
+        v3 = get_local ptr { u64, u64, u64, u64 }, __anon_468
+        v4 = const u64 0
+        v5 = get_elem_ptr v3, ptr u64, v4
+        mem_copy_val v5, v2
+        v6 = load v2
+        ret u64 v6
+    }
+}
+
+// regex: VAL=v\d+
+
+// not: mem_copy_val $VAL, VAL
diff --git a/sway-ir/tests/dce/copy_prop_2.ir b/sway-ir/tests/dce/copy_prop_2.ir
@@ -0,0 +1,21 @@
+script {
+    entry fn main() -> u64 {
+        local { u64, u64, u64, u64 } __anon_0
+        local { u64, u64, u64, u64 } __anon_468
+
+        entry():
+        v0 = get_local ptr { u64, u64, u64, u64 }, __anon_0
+        v1 = get_local ptr { u64, u64, u64, u64 }, __anon_468
+        mem_copy_val v1, v0
+        v2 = const u64 0
+        v3 = get_elem_ptr v1, ptr u64, v2
+        v4 = get_local ptr { u64, u64, u64, u64 }, __anon_0
+        v5 = get_elem_ptr v4, ptr u64, v2
+        v6 = load v5
+        ret u64 v6
+    }
+}
+
+// regex: VAL=v\d+
+
+// not: mem_copy_val $VAL, $VAL
diff --git a/sway-ir/tests/dce/copy_prop_3.ir b/sway-ir/tests/dce/copy_prop_3.ir
@@ -0,0 +1,22 @@
+script {
+    entry fn main(v101: u64) -> u64 {
+        local [u64; 8] __anon_0
+        local [u64; 8] __anon_468
+
+        entry(v101: u64):
+        v0 = get_local ptr [u64; 8], __anon_0
+        v1 = get_local ptr [u64; 8], __anon_468
+        mem_copy_val v1, v0
+        v2 = const u64 1
+        v3 = add v101, v2
+        v4 = get_elem_ptr v1, ptr u64, v3
+        v5 = get_local ptr [u64; 8], __anon_0
+        v6 = get_elem_ptr v5, ptr u64, v3
+        v7 = load v6
+        ret u64 v7
+    }
+}
+
+// regex: VAL=v\d+
+
+// not: mem_copy_val $VAL, $VAL
diff --git a/sway-ir/tests/memcpyopt/copy_prop_1.ir b/sway-ir/tests/memcpyopt/copy_prop_1.ir
@@ -0,0 +1,23 @@
+script {
+    entry fn main() -> u64 {
+        local { u64, u64, u64, u64 } __anon_468
+        local { u64, u64, u64, u64 } __anon_0
+
+        entry():
+        v13 = get_local ptr { u64, u64, u64, u64 }, __anon_0
+        v14 = const u64 0
+        v15 = get_elem_ptr v13, ptr u64, v14
+        v25 = get_local ptr { u64, u64, u64, u64 }, __anon_468
+        v26 = const u64 0
+        v27 = get_elem_ptr v25, ptr u64, v26
+        mem_copy_val v27, v15
+        v0 = load v27
+        ret u64 v0
+    }
+}
+
+// regex: VAL=v\d+
+
+// check: mem_copy_val $(mem_cpy_dest=$VAL), $(mem_cpy_src=$VAL)
+// The optimization should replace the load's source to be the memcpy's source.
+// check: $VAL = load $(mem_cpy_src)
diff --git a/sway-ir/tests/memcpyopt/copy_prop_2.ir b/sway-ir/tests/memcpyopt/copy_prop_2.ir
@@ -0,0 +1,23 @@
+script {
+    entry fn main() -> u64 {
+        local { u64, u64, u64, u64 } __anon_468
+        local { u64, u64, u64, u64 } __anon_0
+
+        entry():
+        v13 = get_local ptr { u64, u64, u64, u64 }, __anon_0
+        v25 = get_local ptr { u64, u64, u64, u64 }, __anon_468
+        mem_copy_val v25, v13
+        v26 = const u64 0
+        v27 = get_elem_ptr v25, ptr u64, v26
+        v0 = load v27
+        ret u64 v0
+    }
+}
+
+// regex: VAL=v\d+
+
+// check: mem_copy_val $VAL, $VAL
+// The optimization should introduce a GEP into __anon_0
+// check: $(local=$VAL) = get_local ptr { u64, u64, u64, u64 }, __anon_0
+// check: $(gep=$VAL) = get_elem_ptr $local, ptr u64, $VAL
+// check: $VAL = load $gep
diff --git a/sway-ir/tests/memcpyopt/copy_prop_3.ir b/sway-ir/tests/memcpyopt/copy_prop_3.ir
@@ -0,0 +1,25 @@
+script {
+    entry fn main(v101: u64) -> u64 {
+        local [u64; 8] __anon_468
+        local [u64; 8] __anon_0
+
+        entry(v101: u64):
+        v13 = get_local ptr [u64; 8], __anon_0
+        v25 = get_local ptr [u64; 8], __anon_468
+        mem_copy_val v25, v13
+        v26 = const u64 1
+        vidx = add v101, v26
+        v27 = get_elem_ptr v25, ptr u64, vidx
+        v0 = load v27
+        ret u64 v0
+    }
+}
+
+// regex: VAL=v\d+
+
+// check: mem_copy_val $VAL, $VAL
+// The optimization should introduce a GEP into __anon_0
+// check: $(idx=$VAL) = add $VAL, $VAL
+// check: $(local=$VAL) = get_local ptr [u64; 8], __anon_0
+// check: $(gep=$VAL) = get_elem_ptr $local, ptr u64, $idx
+// check: $VAL = load $gep
diff --git a/...ests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw b/...ests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw
@@ -2,7 +2,7 @@ script;
 use basic_storage_abi::{BasicStorage, Quad};
 
 fn main() -> u64 {
-    let addr = abi(BasicStorage, 0xa837c1b6bcd9f68bd9164909345f0c52ac223adbae050796551f58ddf0700c27);
+    let addr = abi(BasicStorage, 0x30a664b11896792cc65021bc1253260a7b6f4c110e5e79a5cb0bd11f72adc59c);
     let key = 0x0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
     let value = 4242;