Richard Sandiford <rdsandif...@googlemail.com> writes:
> Olivier Hainque <hain...@adacore.com> writes:
>> Hello Richard,
>>
>> Re $subject, at http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01515.html
>>
>> You suggested:
>>>> Would be nice to use a single function that knows about the extra
>>>> contraints here. Maybe something like the attached?
>>
>> << 2012-04-24 ...
>>
>> * rtl.h (set_for_reg_notes): Declare.
>> * emit-rtl.c (set_for_reg_notes): New function.
>> (set_unique_reg_note): Use it.
>> * optabs.c (add_equal_note): Likewise.
>>>>
>>
>> I had answered:
>>> Looks cleaner indeed. Do you want me to test ?
>>
>> I gave it a try. Your patch bootstraps and regtests fine on mainline for
>> x86-linux.
>
> Sorry, was going to test this earlier, but got distracted by
> lower-subreg stuff. I need to fix the subreg handling so that
> we check whether the inner part of a SUBREG is a REG (done in
> my copy at home). I also wanted to make sure there were no
> asm differences due to notes being wrongly dropped.
So I tried compiling some recent cc1 .ii files on x86_64 at -O2.
The only differences were in fixed-value.ii. In this case we
used to create:
---------------------------------------------------------------------------
(insn 899 898 900 68 (parallel [
(set (reg/f:DI 597)
(plus:DI (reg/f:DI 20 frame)
(const_int -32 [0xffffffffffffffe0])))
(clobber (reg:CC 17 flags))
]) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 252 {*adddi_1}
(nil))
(insn 900 899 901 72 (parallel [
(set (reg:DI 598)
(plus:DI (reg:DI 597)
(const_int 8 [0x8])))
(clobber (reg:CC 17 flags))
]) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 -1
(nil))
(insn 901 900 902 72 (set (mem/f:DI (plus:DI (reg/f:DI 56 virtual-outgoing-args)
(const_int 24 [0x18])) [0 S8 A64])
(reg:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 -1
(expr_list:REG_EQUAL (plus:DI (reg:DI 597)
(const_int 8 [0x8]))
(nil)))
[...other uses of 597...]
(insn 921 920 922 73 (parallel [
(set (reg:DI 604)
(plus:DI (reg:DI 603)
(const_int 8 [0x8])))
(clobber (reg:CC 17 flags))
]) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:580 -1
(nil))
---------------------------------------------------------------------------
where 901 has a REG_EQUAL note against a MEM. cse1 changes the note to:
---------------------------------------------------------------------------
(insn 901 900 902 68 (set (mem/f:DI (plus:DI (reg/f:DI 7 sp)
(const_int 24 [0x18])) [0 S8 A64])
(reg/f:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 62
{*movdi_internal_rex64}
(expr_list:REG_EQUAL (plus:DI (reg/f:DI 20 frame)
(const_int -24 [0xffffffffffffffe8]))
(nil)))
---------------------------------------------------------------------------
but doesn't touch 921 (probably worth finding out why). cse2 then sees
this note and records it as having the same value as both the MEM and
reg 598. So when it comes to insn 921 and replaces the source with reg 598,
it also adds a note:
---------------------------------------------------------------------------
(insn 921 1285 939 71 (set (reg/f:DI 698)
(reg/f:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:580 62
{*movdi_internal_rex64}
(expr_list:REG_EQUAL (plus:DI (reg/f:DI 20 frame)
(const_int -24 [0xffffffffffffffe8]))
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil))))
---------------------------------------------------------------------------
Reload is then able to use this information to drop the 698 and
rematerialise it where necessary. After the patch we just get:
---------------------------------------------------------------------------
(insn 921 1285 939 71 (set (reg/f:DI 698)
(reg/f:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:580 62
{*movdi_internal_rex64}
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)))
---------------------------------------------------------------------------
The problem here seems to be some inconsistency about what is considered
"constant" in cse. The condition for table elements is:
elt->is_const = (CONSTANT_P (x) || fixed_base_plus_p (x));
But the condition for values that been calculated through folding
(e.g. because no note exists) is:
if (src_const == 0
&& (CONSTANT_P (src_folded)
/* Consider (minus (label_ref L1) (label_ref L2)) as
"constant" here so we will record it. This allows us
to fold switch statements when an ADDR_DIFF_VEC is used. */
|| (GET_CODE (src_folded) == MINUS
&& GET_CODE (XEXP (src_folded, 0)) == LABEL_REF
&& GET_CODE (XEXP (src_folded, 1)) == LABEL_REF)))
src_const = src_folded, src_const_elt = elt;
fixed_base_plus_p has some old code related to rtl inlining,
so these days I think the first condition should be equivalent to:
elt->is_const = function_invariant_p (x);
I'm not sure why fixed_plus_base_p checks fixed_regs[ARG_POINTER_REGNUM]
though. (function_invariant_p doesn't.)
If we change the second to also include fixed_base_plus_p/
function_invariant_p:
if (src_const == 0
&& (function_invariant_p (src_folded)
/* Consider (minus (label_ref L1) (label_ref L2)) as
"constant" here so we will record it. This allows us
to fold switch statements when an ADDR_DIFF_VEC is used. */
|| (GET_CODE (src_folded) == MINUS
&& GET_CODE (XEXP (src_folded, 0)) == LABEL_REF
&& GET_CODE (XEXP (src_folded, 1)) == LABEL_REF)))
src_const = src_folded, src_const_elt = elt;
then we get the note back. That in itself isn't a complete patch,
but it makes a surprising difference. I've filed 53260 to record this.
Even though the REG_EQUAL note above is invalid (and documented as
being invalid), I'm guessing people wouldn't want the patch to be
applied until this is sorted out.
Richard
