[Patch, MIPS] Patch to fix MIPS optimization bug in combine

Steve Ellcey sellcey@imgtec.com
Wed Oct 21 16:49:00 GMT 2015


A bug was reported against the GCC MIPS64 compiler that involves a bad combine
and this patch fixes the bug.

When using '-fexpensive-optimizations -march=mips64r2 -mabi=64' GCC is
combining these instructions:

(insn 13 12 14 2 (set (reg:DI 206 [ *last_3(D)+-4 ])
        (zero_extend:DI (subreg/s/u:SI (reg:DI 196 [ *last_3(D)+-4 ]) 4))) x.c:21 212 {*zero_extendsidi2_dext}
     (nil))

(insn 15 14 16 2 (set (reg:DI 208)
        (and:DI (reg:DI 207)
            (const_int 4294967295 [0xffffffff]))) x.c:21 182 {*anddi3}
     (expr_list:REG_DEAD (reg:DI 207)
        (nil)))

(jump_insn 16 15 17 2 (set (pc)
        (if_then_else (ne (reg:DI 206 [ *last_3(D)+-4 ])
                (reg:DI 208))
            (label_ref:DI 35)
            (pc))) x.c:21 473 {*branch_equalitydi}
     (expr_list:REG_DEAD (reg:DI 208)
        (expr_list:REG_DEAD (reg:DI 206 [ *last_3(D)+-4 ])
            (int_list:REG_BR_PROB 8010 (nil))))
 -> 35)

Into:

(jump_insn 16 15 17 2 (set (pc)
        (if_then_else (ne (subreg:SI (reg:DI 207) 4)
                (subreg:SI (reg:DI 196 [ *last_3(D)+-4 ]) 4))
            (label_ref:DI 35)
            (pc))) x.c:21 472 {*branch_equalitysi}
     (expr_list:REG_DEAD (reg:DI 207)
        (int_list:REG_BR_PROB 8010 (nil)))
 -> 35)


The problem is that the jump_insn on MIPS64 generates a beq/bne instruction
that compares the entire 64 bit registers and there is no option to only
look at the bottom 32 bits.  When we got rid of insn 15 we lost the AND that
cleared the upper 32 bits of one of the registers and the program fails.

My solution was to not allow subregs in the conditional jump instruction.
Here is the patch and a test case and I ran the GCC testsuite with no
regressions.

OK to checkin?

Steve Ellcey
sellcey@imgtec.com


2015-10-21  Steve Ellcey  <sellcey@imgtec.com>

	* mips.c (mips_legitimate_combined_insn): New function.
	(TARGET_LEGITIMATE_COMBINED_INSN): New macro.


diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 0b4a5fa..4338628 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -19606,6 +19606,26 @@ mips_ira_change_pseudo_allocno_class (int regno, reg_class_t allocno_class)
     return GR_REGS;
   return allocno_class;
 }
+
+/* Implement TARGET_LEGITIMATE_COMBINED_INSN */
+
+static bool
+mips_legitimate_combined_insn (rtx_insn* insn)
+{
+  /* If we do a conditional jump involving register compares do not allow
+     subregs because beq/bne will always compare the entire register.
+     This should only be an issue with N32/N64 ABI's that do a 32 bit
+     compare and jump.  */
+
+  if (any_condjump_p (insn))
+    {
+      rtx cond = XEXP (SET_SRC (pc_set (insn)), 0);
+      if (GET_RTX_CLASS (GET_CODE (cond)) == RTX_COMPARE
+	  || GET_RTX_CLASS (GET_CODE (cond)) == RTX_COMM_COMPARE)
+	return !SUBREG_P (XEXP (cond, 0)) && !SUBREG_P (XEXP (cond, 1));
+    }
+  return true;
+}
 
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
@@ -19868,6 +19888,9 @@ mips_ira_change_pseudo_allocno_class (int regno, reg_class_t allocno_class)
 #undef TARGET_HARD_REGNO_SCRATCH_OK
 #define TARGET_HARD_REGNO_SCRATCH_OK mips_hard_regno_scratch_ok
 
+#undef TARGET_LEGITIMATE_COMBINED_INSN
+#define TARGET_LEGITIMATE_COMBINED_INSN mips_legitimate_combined_insn
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-mips.h"





2015-10-21  Steve Ellcey  <sellcey@imgtec.com>

	* gcc.dg/combine-subregs.c: New test.


diff --git a/gcc/testsuite/gcc.dg/combine-subregs.c b/gcc/testsuite/gcc.dg/combine-subregs.c
index e69de29..c480f88 100644
--- a/gcc/testsuite/gcc.dg/combine-subregs.c
+++ b/gcc/testsuite/gcc.dg/combine-subregs.c
@@ -0,0 +1,36 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fexpensive-optimizations" } */
+
+#include <inttypes.h>
+#include <stdlib.h>
+
+void __attribute__ ((noinline))
+foo (uint64_t state, uint32_t last)
+{
+  if (state == last) abort ();
+}
+
+/* This function may do a bad comparision by trying to
+   use SUBREGS during the compare on machines where comparing
+   two registers always compares the entire register regardless
+   of mode.  */
+
+int __attribute__ ((noinline))
+compare (uint64_t state, uint32_t *last, uint8_t buf)
+{
+    if (*last == ((state | buf) & 0xFFFFFFFF)) {
+	foo (state, *last);
+        return 0;
+    }
+    return 1;
+}
+
+int
+main(int argc, char **argv) {
+    uint64_t state = 0xF00000100U;
+    uint32_t last  = 0x101U;
+    int ret        = compare(state, &last, 0x01);
+    if (ret != 0)
+	abort ();
+    exit (0);
+}



More information about the Gcc-patches mailing list