From eebae8d4c8f847a353e44679abfd0a2f74bc3944 Mon Sep 17 00:00:00 2001
From: Afonso Bordado <afonsobordado@az8.co>
Date: Sat, 3 Jul 2021 12:32:22 +0100
Subject: [PATCH] aarch64: Fix incorrect encoding of large const values in
 icmp.

When encoding constants as immediates into an RSE Imm12 instruction we need to take special care to check if the value that we are trying to input does not overflow its type when viewed as a signed value. (i.e. iconst.i8 200)

We cannot both put an immediate and sign extend it, so we need to lower it into a separate reg, and emit the sign extend into the instruction.

For more details see the [cg_clif bug report](https://github.com/bjorn3/rustc_codegen_cranelift/issues/1184#issuecomment-873214796).
---
 cranelift/codegen/src/isa/aarch64/lower.rs    | 19 ++++++++++++++++++-
 .../filetests/filetests/runtests/icmp.clif    | 17 +++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 cranelift/filetests/filetests/runtests/icmp.clif

diff --git a/cranelift/codegen/src/isa/aarch64/lower.rs b/cranelift/codegen/src/isa/aarch64/lower.rs
index 9a156fc90d..4e460496df 100644
--- a/cranelift/codegen/src/isa/aarch64/lower.rs
+++ b/cranelift/codegen/src/isa/aarch64/lower.rs
@@ -24,6 +24,7 @@ use crate::data_value::DataValue;
 use log::{debug, trace};
 use regalloc::{Reg, Writable};
 use smallvec::SmallVec;
+use std::cmp;
 
 //============================================================================
 // Result enum types.
@@ -156,6 +157,14 @@ impl NarrowValueMode {
             NarrowValueMode::ZeroExtend64 | NarrowValueMode::SignExtend64 => false,
         }
     }
+
+    fn is_signed(&self) -> bool {
+        match self {
+            NarrowValueMode::SignExtend32 | NarrowValueMode::SignExtend64 => true,
+            NarrowValueMode::ZeroExtend32 | NarrowValueMode::ZeroExtend64 => false,
+            NarrowValueMode::None => false,
+        }
+    }
 }
 
 /// Emits instruction(s) to generate the given constant value into newly-allocated
@@ -438,7 +447,15 @@ pub(crate) fn put_input_in_rse_imm12<C: LowerCtx<I = Inst>>(
 ) -> ResultRSEImm12 {
     if let Some(imm_value) = input_to_const(ctx, input) {
         if let Some(i) = Imm12::maybe_from_u64(imm_value) {
-            return ResultRSEImm12::Imm12(i);
+            let out_ty_bits = ty_bits(ctx.input_ty(input.insn, input.input));
+            let is_negative = (i.bits as u64) & (1 << (cmp::max(out_ty_bits, 1) - 1)) != 0;
+
+            // This condition can happen if we matched a value that overflows the output type of
+            // its `iconst` when viewed as a signed value (i.e. iconst.i8 200).
+            // When that happens we need to lower as a negative value, which we cannot do here.
+            if !(narrow_mode.is_signed() && is_negative) {
+                return ResultRSEImm12::Imm12(i);
+            }
         }
     }
 
diff --git a/cranelift/filetests/filetests/runtests/icmp.clif b/cranelift/filetests/filetests/runtests/icmp.clif
new file mode 100644
index 0000000000..21c0c4d873
--- /dev/null
+++ b/cranelift/filetests/filetests/runtests/icmp.clif
@@ -0,0 +1,17 @@
+test run
+target aarch64
+target s390x
+target x86_64 machinst
+
+; This test is also a regression test for aarch64.
+; We were not correctly handling the fact that the rhs constant value
+; overflows its type when viewed as a signed value, and thus encoding the wrong
+; value into the resulting instruction.
+function %overflow_rhs_const(i8) -> b1 {
+block0(v0: i8):
+    v1 = iconst.i8 192
+    v2 = icmp sge v0, v1
+    return v2
+}
+; run: %test(49) == true
+; run: %test(-65) == false