On Wed, Nov 28, 2012 at 11:10 PM, Eric Botcazou wrote:
>> Well, I'm not sure I agree that they are wrong. Consider:
>>
>> S0: r1 = ...
>> S1: r2 = r1 + 10
>> S2: r1 = r2 + 10 { REG_EQUAL r1 + 20 }
>> S3: r3 = r1 + 10
>>
>> Clearly it would be wrong to use the REG_EQUAL note on S2 to optimize S3 to:
>>
>> S0: r1 = ...
>> S1: r2 = r1 + 10
>> S2: r1 = r1 + 20
>> S3: r3 = r1 + 30
>
> But the note is wrong by itself. The semantics is clear: the note means that
> r1 is equal to r1 + 20 right after S2, which doesn't make any sense.
Or maybe the semantics are not what the compiler is actually doing.
Because clearly several places in the compiler can create this kind of
self-referencing note right now, and the main consumer of the notes,
CSE, has apparently not had too much trouble handing them.
But with the documented semantics, you're right. And, to be honest,
I'm of the "the fewer notes, the better" camp :-)
>> 2: debug_rtx((rtx)0x7ffff4df5e58) = (insn 638 637 639 85 (parallel [
>> (set (reg:SI 891)
>> (minus:SI (reg:SI 890)
>> (reg:SI 546 [ D.45472 ])))
>> (clobber (reg:CC 17 flags))
>> ]) ../../trunk/gcc/value-prof.c:928 283 {*subsi_1}
>> (expr_list:REG_EQUIV (mem:SI (plus:DI (reg/v/f:DI 204 [ e12 ])
>> (const_int 52 [0x34])) [4 e12_223->probability+0 S4 A32])
>> (expr_list:REG_UNUSED (reg:CC 17 flags)
>> (expr_list:REG_EQUAL (minus:SI (const_int 10000 [0x2710])
>> (reg:SI 546 [ D.45472 ]))
>> (nil)))))
>> void
>> (gdb)
>>
>> Is that valid?
>
> Yes, the comment is wrong and should have been removed in r183719:
> http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01547.html
Ugh, that doesn't look like a very solid fix.
Attached is a different fix. It splits DF_REF_IN_NOTE in two: One flag
for each kind of note. This allows the dead note removal code to
distinguish the source note for the EQ_USES. I needed to remove one
flag to keep the df_ref_flags 16-bit, but the DF_REF_SUBREG flag looks
completely unnecessary to me, and only ira.c uses it, so it wasn't to
hard to scavenge a single bit.
The patch also includes all places I've found so far where the
compiler could create self-referencing notes:
1. optabs.c: Not sure what it was trying to do, but now it just
refuses to add a note if TARGET is mentioned in one of the source
operands.
2. gcse.c: gcse_emit_move_after added notes, but none of them was very
useful as far as I could tell, and almost all of them turned
self-referencing after CPROP. So I propose we just never add notes in
this case.
3. cprop.c: It seems to me that the purpose here is to propagate
constants. If a reg could not be propagated, then the REG_EQUAL note
will not help much either. Propagating constants via REG_EQUAL notes
still helps folding comparisons sometimes, so I'm proposing we only
propagate those. As a bonus: less garbage RTL because a
cprop_constant_p can be shared.
4. fwprop.c: If the SET_DEST is a REG that is mentioned in the
SET_SRC, don' create a note. This one I'm not very happy with, because
it doesn't handle the case that the REG is somehow simplified out of
the SET_SRC, but being smarter about this would only complicate
things. I for one can't think of anything better for the moment,
anyway.
Finally, it makes sense to compute the NOTE problem for CSE.
Bootstrap&testing in progress at the older revision I've been using to
debug the darwin10 and sparc bugs. I'll test trunk head on x86_64 and
powerpc later. In the mean time, let me hear what you think of this
one :-)
Ciao!
Steven
PR55006_2.diff
Description: Binary data
