Bugfix: Strength reduce must not optimize mul into shl if overflow (h…

…yperledger#1452) Optimizing multiplications with a SHL must not disable overflow checks. Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
xermicus · Jul 22, 2023 · 76c0109 · 76c0109
1 parent 7adce9b
commit 76c0109
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 30 deletions.
diff --git a/src/codegen/strength_reduce/mod.rs b/src/codegen/strength_reduce/mod.rs
@@ -230,38 +230,41 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
                     let left_values = expression_values(left, vars, ns);
                     let right_values = expression_values(right, vars, ns);
 
-                    if let Some(right) = is_single_constant(&right_values) {
-                        // is it a power of two
-                        // replace with a shift
-                        let mut shift = BigInt::one();
-                        let mut cmp = BigInt::from(2);
-
-                        for _ in 1..bits {
-                            if cmp == right {
-                                ns.hover_overrides.insert(
-                                    *loc,
-                                    format!(
-                                        "{} multiply optimized to shift left {}",
-                                        ty.to_string(ns),
-                                        shift
-                                    ),
-                                );
+                    match is_single_constant(&right_values) {
+                        Some(right) if *overflowing => {
+                            // is it a power of two
+                            // replace with a shift
+                            let mut shift = BigInt::one();
+                            let mut cmp = BigInt::from(2);
+
+                            for _ in 1..bits {
+                                if cmp == right {
+                                    ns.hover_overrides.insert(
+                                        *loc,
+                                        format!(
+                                            "{} multiply optimized to shift left {}",
+                                            ty.to_string(ns),
+                                            shift
+                                        ),
+                                    );
 
-                                return Expression::ShiftLeft {
-                                    loc: *loc,
-                                    ty: ty.clone(),
-                                    left: left.clone(),
-                                    right: Box::new(Expression::NumberLiteral {
+                                    return Expression::ShiftLeft {
                                         loc: *loc,
                                         ty: ty.clone(),
-                                        value: shift,
-                                    }),
-                                };
+                                        left: left.clone(),
+                                        right: Box::new(Expression::NumberLiteral {
+                                            loc: *loc,
+                                            ty: ty.clone(),
+                                            value: shift,
+                                        }),
+                                    };
+                                }
+
+                                cmp *= 2;
+                                shift += 1;
                             }
-
-                            cmp *= 2;
-                            shift += 1;
                         }
+                        _ => (), // SHL would disable overflow check
                     }
 
                     if ty.is_signed_int(ns) {

diff --git a/tests/codegen_testcases/solidity/strength_reduce.sol b/tests/codegen_testcases/solidity/strength_reduce.sol
@@ -43,10 +43,17 @@ contract test {
 // BEGIN-CHECK: test::function::f4
     function f4() pure public {
         for (uint i = 0; i < 10; i++) {
-            // this multiply can be done with a 64 bit instruction
-            print("i:{}".format(i * 32768));
+            unchecked {
+                // this multiply can be done with a 64 bit instruction
+                print("i:{}".format(i * 32768));
+            }
         }
 // CHECK: (%i << uint256 15)
+        for (uint i = 0; i < 10; i++) {
+            // Do not disable overflow checks
+            print("i:{}".format(i * 32768));
+        }
+// NOT CHECK: (%i << uint256 15)
     }
 
 // BEGIN-CHECK: test::function::f5
@@ -114,6 +121,6 @@ contract test {
         for (int i = 0; i != 102; i++) {
             print("i:{}".format(i % 0x1_0000_0001));
         }
-// CHECK: (signed modulo %i % int256 4294967297)
+                // CHECK: (signed modulo %i % int256 4294967297)
     }
 }