This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



> On Oct 22, 2015, at 12:44 AM, Steve Ellcey <sellcey@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
> 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);
> +}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]