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