| Message ID | 3219649.lQJTE2xl1Q@polaris |
|---|---|
| State | New |
| Headers | show |
On 10/24/13 15:28, Eric Botcazou wrote: > This is a very old bug in the alias.c machinery, which is exposed during > the sched2 pass. We have in .split4: > > (insn 23 22 24 4 (set (mem/f:DI (reg/v/f:DI 4 si [orig:90 p2 ] [90]) [3 > *p2_9(D)+0 S8 A64]) > (symbol_ref:DI ("d") <var_decl 0x7ffff6e42720 d>)) pr58831-1.c:12 85 > {*movdi_internal} > (expr_list:REG_DEAD (reg/v/f:DI 4 si [orig:90 p2 ] [90]) > (nil))) > > [...] > > (insn 61 27 62 6 (clobber (reg:DI 4 si)) -1 > (nil)) > > (insn/f 62 61 56 6 (parallel [ > (set (mem:DI (pre_dec:DI (reg/f:DI 7 sp)) [0 S8 A8]) > (reg:DI 4 si)) > (clobber (mem:BLK (scratch) [0 A8])) > ]) pr58831-1.c:8 73 {*pushdi2_prologue} > (expr_list:REG_CFA_ADJUST_CFA (set (reg/f:DI 7 sp) > (plus:DI (reg/f:DI 7 sp) > (const_int -8 [0xfffffffffffffff8]))) > (nil))) > > [...] > > (insn 30 29 31 6 (set (reg:DI 4 si) > (symbol_ref/f:DI ("*.LC0") [flags 0x2] <var_decl 0x7ffff6e72098 > *.LC0>)) pr58831-1.c:14 85 {*movdi_internal} > (nil)) > > si is the 2nd argument register on x86-64 so it's live on entry. The problem > is that the clobber at insn 61 wipes out the value recorded for si: > > /* A CLOBBER wipes out any old value but does not prevent a previously > unset register from acquiring a base address (i.e. reg_seen is not > set). */ > if (GET_CODE (set) == CLOBBER) > { > new_reg_base_value[regno] = 0; > return; > } > > by init_alias_target: > > for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) > /* Check whether this register can hold an incoming pointer > argument. FUNCTION_ARG_REGNO_P tests outgoing register > numbers, so translate if necessary due to register windows. */ > if (FUNCTION_ARG_REGNO_P (OUTGOING_REGNO (i)) > && HARD_REGNO_MODE_OK (i, Pmode)) > static_reg_base_value[i] = arg_base_value; > > and later copied by init_alias_analysis: > > /* Mark all hard registers which may contain an address. > The stack, frame and argument pointers may contain an address. > An argument register which can hold a Pmode value may contain > an address even if it is not in BASE_REGS. > > The address expression is VOIDmode for an argument and > Pmode for other registers. */ > > memcpy (new_reg_base_value, static_reg_base_value, > FIRST_PSEUDO_REGISTER * sizeof (rtx)); > > and that nothing indicates that there was a valid value before the clobber, so > the machinery wrongly records the set at insn 30. I think the fix is just to > set reg_seen[N] if static_reg_base_value[N] is non-null in the copy above made > by init_alias_analysis. Tested on x86_64-suse-linux. > > Jeff, it's old code of yours, are you OK with this? Actually I think it's jfc's code; he had a major revamp of alias.c back in the early egcs days and getting it integrated was one of the project's early wins. Sadly, jfc hasn't been involved in GCC work for a long long time. Anyway, I think the issue here is that once we set new_reg_base_value[regno] to zero, when we do encounter insn 30, we assume that's the first set of (reg 4) and we use the RHS as a base value. The result is we end up with something in new_reg_base_value[4] that is not correct for the entire life. ie, from function entry to the clobber (reg 4) has a value totally unrelated to what we've stored in new_reg_base_value[4]? RIght? My memory of all this is fuzzy, but this code in alias.c is effectively trying to compute a set of flow independent base values for each register. If we have two unrelated values going into a register, then we need to essentially say we know nothing about its address and the aliases that creates. > > > 2013-10-24 Eric Botcazou <ebotcazou@adacore.com> > > PR rtl-optimization/58831 > * alias.c (init_alias_analysis): At the beginning of each iteration, set > the reg_seen[N] bit if static_reg_base_value[N] is non-null. > > > 2013-10-24 Eric Botcazou <ebotcazou@adacore.com> > > * gcc.c-torture/execute/pr58831.c: New test. Seems reasonable to me, assuming my understandings noted above are correct. jeff
> Actually I think it's jfc's code; he had a major revamp of alias.c back > in the early egcs days and getting it integrated was one of the > project's early wins. Sadly, jfc hasn't been involved in GCC work for a > long long time. OK, thanks for the clarification. > Anyway, I think the issue here is that once we set > new_reg_base_value[regno] to zero, when we do encounter insn 30, we > assume that's the first set of (reg 4) and we use the RHS as a base value. > > The result is we end up with something in new_reg_base_value[4] that is > not correct for the entire life. ie, from function entry to the clobber > (reg 4) has a value totally unrelated to what we've stored in > new_reg_base_value[4]? RIght? Yes, that's exactly it. > Seems reasonable to me, assuming my understandings noted above are correct. Thanks.
Index: alias.c =================================================================== --- alias.c (revision 203876) +++ alias.c (working copy) @@ -2975,16 +2975,13 @@ init_alias_analysis (void) /* Wipe the reg_seen array clean. */ bitmap_clear (reg_seen); - /* Mark all hard registers which may contain an address. - The stack, frame and argument pointers may contain an address. - An argument register which can hold a Pmode value may contain - an address even if it is not in BASE_REGS. - - The address expression is VOIDmode for an argument and - Pmode for other registers. */ - - memcpy (new_reg_base_value, static_reg_base_value, - FIRST_PSEUDO_REGISTER * sizeof (rtx)); + /* Initialize the alias information for this pass. */ + for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) + if (static_reg_base_value[i]) + { + new_reg_base_value[i] = static_reg_base_value[i]; + bitmap_set_bit (reg_seen, i); + } /* Walk the insns adding values to the new_reg_base_value array. */ for (i = 0; i < rpo_cnt; i++)
This is a very old bug in the alias.c machinery, which is exposed during the sched2 pass. We have in .split4: (insn 23 22 24 4 (set (mem/f:DI (reg/v/f:DI 4 si [orig:90 p2 ] [90]) [3 *p2_9(D)+0 S8 A64]) (symbol_ref:DI ("d") <var_decl 0x7ffff6e42720 d>)) pr58831-1.c:12 85 {*movdi_internal} (expr_list:REG_DEAD (reg/v/f:DI 4 si [orig:90 p2 ] [90]) (nil))) [...] (insn 61 27 62 6 (clobber (reg:DI 4 si)) -1 (nil)) (insn/f 62 61 56 6 (parallel [ (set (mem:DI (pre_dec:DI (reg/f:DI 7 sp)) [0 S8 A8]) (reg:DI 4 si)) (clobber (mem:BLK (scratch) [0 A8])) ]) pr58831-1.c:8 73 {*pushdi2_prologue} (expr_list:REG_CFA_ADJUST_CFA (set (reg/f:DI 7 sp) (plus:DI (reg/f:DI 7 sp) (const_int -8 [0xfffffffffffffff8]))) (nil))) [...] (insn 30 29 31 6 (set (reg:DI 4 si) (symbol_ref/f:DI ("*.LC0") [flags 0x2] <var_decl 0x7ffff6e72098 *.LC0>)) pr58831-1.c:14 85 {*movdi_internal} (nil)) si is the 2nd argument register on x86-64 so it's live on entry. The problem is that the clobber at insn 61 wipes out the value recorded for si: /* A CLOBBER wipes out any old value but does not prevent a previously unset register from acquiring a base address (i.e. reg_seen is not set). */ if (GET_CODE (set) == CLOBBER) { new_reg_base_value[regno] = 0; return; } by init_alias_target: for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) /* Check whether this register can hold an incoming pointer argument. FUNCTION_ARG_REGNO_P tests outgoing register numbers, so translate if necessary due to register windows. */ if (FUNCTION_ARG_REGNO_P (OUTGOING_REGNO (i)) && HARD_REGNO_MODE_OK (i, Pmode)) static_reg_base_value[i] = arg_base_value; and later copied by init_alias_analysis: /* Mark all hard registers which may contain an address. The stack, frame and argument pointers may contain an address. An argument register which can hold a Pmode value may contain an address even if it is not in BASE_REGS. The address expression is VOIDmode for an argument and Pmode for other registers. */ memcpy (new_reg_base_value, static_reg_base_value, FIRST_PSEUDO_REGISTER * sizeof (rtx)); and that nothing indicates that there was a valid value before the clobber, so the machinery wrongly records the set at insn 30. I think the fix is just to set reg_seen[N] if static_reg_base_value[N] is non-null in the copy above made by init_alias_analysis. Tested on x86_64-suse-linux. Jeff, it's old code of yours, are you OK with this? 2013-10-24 Eric Botcazou <ebotcazou@adacore.com> PR rtl-optimization/58831 * alias.c (init_alias_analysis): At the beginning of each iteration, set the reg_seen[N] bit if static_reg_base_value[N] is non-null. 2013-10-24 Eric Botcazou <ebotcazou@adacore.com> * gcc.c-torture/execute/pr58831.c: New test.