From 52256eb4319fe04e05ebb12a57ca9ef73c5e729d Mon Sep 17 00:00:00 2001
From: Yoshitomo Nakanishi <yurayura.rounin.3@gmail.com>
Date: Fri, 27 May 2022 14:13:44 +0900
Subject: [PATCH] Fix wrong safemath lowering

---
 crates/codegen/src/yul/isel/function.rs | 19 +++++++++++++++----
 crates/mir/src/ir/value.rs              |  4 ++++
 newsfragments/723.bugfix.md             |  2 ++
 3 files changed, 21 insertions(+), 4 deletions(-)
 create mode 100644 newsfragments/723.bugfix.md

diff --git a/crates/codegen/src/yul/isel/function.rs b/crates/codegen/src/yul/isel/function.rs
index 46f60c5c9e..0c6eabc7f8 100644
--- a/crates/codegen/src/yul/isel/function.rs
+++ b/crates/codegen/src/yul/isel/function.rs
@@ -455,6 +455,7 @@ impl<'db, 'a> FuncLowerHelper<'db, 'a> {
             }
         )}
     }
+
     fn lower_assign(&mut self, lhs: &AssignableValue, rhs: ValueId) -> yul::Statement {
         match lhs {
             AssignableValue::Value(value) => {
@@ -494,14 +495,23 @@ impl<'db, 'a> FuncLowerHelper<'db, 'a> {
     }
 
     fn lower_unary(&mut self, op: UnOp, value: ValueId) -> yul::Expression {
-        let value = self.value_expr(value);
+        let value_expr = self.value_expr(value);
         match op {
-            UnOp::Not => expression! { iszero([value])},
+            UnOp::Not => expression! { iszero([value_expr])},
             UnOp::Neg => {
                 let zero = literal_expression! {0};
-                expression! { sub([zero], [value])}
+                if self.body.store.value_data(value).is_imm() {
+                    // Literals are checked at compile time (e.g. -128) so there's no point
+                    // in adding a runtime check.
+                    expression! {sub([zero], [value_expr])}
+                } else {
+                    let value_ty = self.body.store.value_ty(value);
+                    self.ctx
+                        .runtime
+                        .safe_sub(self.db, zero, value_expr, value_ty)
+                }
             }
-            UnOp::Inv => expression! { not([value])},
+            UnOp::Inv => expression! { not([value_expr])},
         }
     }
 
@@ -535,6 +545,7 @@ impl<'db, 'a> FuncLowerHelper<'db, 'a> {
             BinOp::Mod => self.ctx.runtime.safe_mod(self.db, lhs, rhs, inst_result_ty),
             BinOp::Pow => self.ctx.runtime.safe_pow(self.db, lhs, rhs, inst_result_ty),
             BinOp::Shl => expression! {shl([rhs], [lhs])},
+            BinOp::Shr if is_signed => expression! {sar([rhs], [lhs])},
             BinOp::Shr => expression! {shr([rhs], [lhs])},
             BinOp::BitOr | BinOp::LogicalOr => expression! {or([lhs], [rhs])},
             BinOp::BitXor => expression! {xor([lhs], [rhs])},
diff --git a/crates/mir/src/ir/value.rs b/crates/mir/src/ir/value.rs
index cdb2d69a8b..48fe24ecc6 100644
--- a/crates/mir/src/ir/value.rs
+++ b/crates/mir/src/ir/value.rs
@@ -42,6 +42,10 @@ impl Value {
             | Self::Constant { ty, .. } => *ty,
         }
     }
+
+    pub fn is_imm(&self) -> bool {
+        matches!(self, Self::Immediate { .. })
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
diff --git a/newsfragments/723.bugfix.md b/newsfragments/723.bugfix.md
new file mode 100644
index 0000000000..6d3f06f066
--- /dev/null
+++ b/newsfragments/723.bugfix.md
@@ -0,0 +1,2 @@
+* Properly lower right shift operation to yul's `sar` if operand is signed type
+* Properly lower negate operation to call `safe_sub`