> On Oct 22, 2015, at 12:44 AM, Steve Ellcey <sell...@imgtec.com> wrote:
>
>
> 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?
No this is the wrong approach. The problem is in combine. I had submitted a
patch to fix a while back but I never got around to the comments to make it
consistent with the rest of combine.
Let me dig up my patch in a few minutes.
Thanks,
Andrew
>
> Steve Ellcey
> sell...@imgtec.com
>
>
> 2015-10-21 Steve Ellcey <sell...@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 <sell...@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);
> +}
