simple_gvn: recognize commutative operators (#6135)

* simple_gvn: recognize commutative operators

Normalize instructions with commutative opcodes by sorting the arguments. This
means instructions like `iadd v0, v1` and `iadd v1, v0` will be considered
identical by GVN and deduplicated.

* Remove `UsubSat` and `SsubSat` from `is_commutative`

They are not actually commutative

* Remove `TODO`s

* Move InstructionData normalization into helper fn

* Add normalization of commutative instructions in the epgrah implementation

* Handle reflexive icmp/fcmps in GVN

* Change formatting of `normalize_in_place`

* suggestions from code review

cranelift/codegen/src/egraph.rs

@@ -134,6 +134,11 @@ where
     /// - Update the value-to-opt-value map, and update the eclass
     ///   union-find, if we rewrote the value to different form(s).
     pub(crate) fn insert_pure_enode(&mut self, inst: NewOrExistingInst) -> Value {
+        match inst {
+            NewOrExistingInst::New(mut data, _) => data.normalize_in_place(),
+            NewOrExistingInst::Existing(inst) => self.func.dfg.insts[inst].normalize_in_place(),
+        }
+
         // Create the external context for looking up and updating the
         // GVN map. This is necessary so that instructions themselves
         // do not have to carry all the references or data for a full

cranelift/codegen/src/ir/condcodes.rs

@@ -32,7 +32,7 @@ pub trait CondCode: Copy {
 /// This condition code is used by the `icmp` instruction to compare integer values. There are
 /// separate codes for comparing the integers as signed or unsigned numbers where it makes a
 /// difference.
-#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 pub enum IntCC {
     /// `==`.
@@ -194,7 +194,7 @@ impl FromStr for IntCC {
 /// The condition codes described here are used to produce a single boolean value from the
 /// comparison. The 14 condition codes here cover every possible combination of the relation above
 /// except the impossible `!UN & !EQ & !LT & !GT` and the always true `UN | EQ | LT | GT`.
-#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 pub enum FloatCC {
     /// EQ | LT | GT

cranelift/codegen/src/ir/instructions.rs

@@ -6,6 +6,7 @@
 //! A large part of this module is auto-generated from the instruction descriptions in the meta
 //! directory.
 
+use crate::ir::condcodes::CondCode;
 use alloc::vec::Vec;
 use core::fmt::{self, Display, Formatter};
 use core::ops::{Deref, DerefMut};
@@ -200,6 +201,32 @@ impl Opcode {
             _ => false,
         }
     }
+
+    /// Returns true if the instruction is commutative: `op(x, y) == op(y, x)` for all x, y
+    pub fn is_commutative(&self) -> bool {
+        match self {
+            Opcode::Band | Opcode::Bor | Opcode::Bxor | Opcode::BxorNot => true,
+
+            Opcode::Iadd
+            | Opcode::Imul
+            | Opcode::IaddPairwise
+            | Opcode::Umulhi
+            | Opcode::Smulhi
+            | Opcode::UaddOverflowTrap
+            | Opcode::Umin
+            | Opcode::Umax
+            | Opcode::Smin
+            | Opcode::Smax
+            | Opcode::AvgRound
+            | Opcode::UaddSat
+            | Opcode::SaddSat => true,
+
+            Opcode::Fadd | Opcode::Fmul => true,
+            Opcode::Fmin | Opcode::Fmax => true,
+
+            _ => false,
+        }
+    }
 }
 
 // This trait really belongs in cranelift-reader where it is used by the `.clif` file parser, but since
@@ -469,6 +496,37 @@ impl InstructionData {
             _ => {}
         }
     }
+
+    /// Normalize commutative instructions by sorting arguments
+    #[inline]
+    pub(crate) fn normalize_in_place(&mut self) {
+        match self {
+            InstructionData::Binary { opcode, args }
+                if args[0] > args[1] && opcode.is_commutative() =>
+            {
+                args.swap(0, 1)
+            }
+            InstructionData::Ternary {
+                opcode: Opcode::Fma,
+                args,
+            } if args[0] > args[1] => args.swap(0, 1),
+            InstructionData::IntCompare { args, cond, .. } if args[0] > args[1] => {
+                args.swap(0, 1);
+                *cond = cond.reverse();
+            }
+            InstructionData::IntCompare { args, cond, .. } if args[0] == args[1] => {
+                *cond = std::cmp::min(*cond, cond.reverse());
+            }
+            InstructionData::FloatCompare { args, cond, .. } if args[0] > args[1] => {
+                args.swap(0, 1);
+                *cond = cond.reverse();
+            }
+            InstructionData::FloatCompare { args, cond, .. } if args[0] == args[1] => {
+                *cond = std::cmp::min(*cond, cond.reverse());
+            }
+            _ => {}
+        }
+    }
 }
 
 /// Information about call instructions.

cranelift/codegen/src/simple_gvn.rs

@@ -35,6 +35,14 @@ struct HashKey<'a, 'f: 'a> {
     ty: Type,
     pos: &'a RefCell<FuncCursor<'f>>,
 }
+
+impl<'a, 'f: 'a> HashKey<'a, 'f> {
+    fn new(mut inst: InstructionData, ty: Type, pos: &'a RefCell<FuncCursor<'f>>) -> Self {
+        inst.normalize_in_place();
+        Self { inst, ty, pos }
+    }
+}
+
 impl<'a, 'f: 'a> Hash for HashKey<'a, 'f> {
     fn hash<H: Hasher>(&self, state: &mut H) {
         let pool = &self.pos.borrow().func.dfg.value_lists;
@@ -113,11 +121,7 @@ pub fn do_simple_gvn(func: &mut Function, domtree: &mut DominatorTree) {
             }
 
             let ctrl_typevar = func.dfg.ctrl_typevar(inst);
-            let key = HashKey {
-                inst: func.dfg.insts[inst],
-                ty: ctrl_typevar,
-                pos: &pos,
-            };
+            let key = HashKey::new(func.dfg.insts[inst], ctrl_typevar, &pos);
             use crate::scoped_hash_map::Entry::*;
             match visible_values.entry(key) {
                 Occupied(entry) => {