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

[Andrew Pinski <pinskia at gmail dot com>]

On Thu, Oct 22, 2015 at 3:32 AM,  <pinskia@gmail.com> wrote:
>
>
>> 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.

So this is recorded as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67736 .
And my patch which I submitted 3 years ago to fix this (though not
fixed up for the comments) is here:
https://gcc.gnu.org/ml/gcc-patches/2012-05/msg00401.html .

Basically combine's simplify_comparison uses gen_lowpart instead of
gen_lowpart_or_truncate but Erir's comment is also true so just update
my patch to Eric's comments instead.

Thanks,
Andrew

>
> 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);
>> +}