On 08/09/14 04:13, Richard Sandiford wrote:
Jeff Law <l...@redhat.com> writes:
On 08/03/14 08:32, Richard Sandiford wrote:
The old for_each_inc_dec callback had a for_each_rtx-like return value,
with >0 being returned directly, 0 meaning "continue" and <0 meaning
"skip subrtxes". But there's no reason to distinguish the latter two
cases since auto-inc/dec expressions aren't allowed to contain other
auto-inc/dec expressions. And if for_each_rtx is going away, there's
no longer any consistency argument for using the same interface.
gcc/
* rtl.h (for_each_inc_dec_fn): Remove special case for -1.
* cselib.c (cselib_record_autoinc_cb): Update accordingly.
(cselib_record_sets): Likewise.
* dse.c (emit_inc_dec_insn_before, check_for_inc_dec_1)
(check_for_inc_dec): Likewise.
* rtlanal.c (for_each_inc_dec_ops): Delete.
(for_each_inc_dec_find_inc_dec): Take the MEM as argument,
rather than a pointer to the memory address. Replace
for_each_inc_dec_ops argument with separate function and data
arguments. Abort on non-autoinc addresses.
(for_each_inc_dec_find_mem): Delete.
(for_each_inc_dec): Use FOR_EACH_SUBRTX_VAR to visit every
autoinc MEM.
So this patch has me a little bit concerned.
@@ -2523,7 +2523,7 @@ cselib_record_sets (rtx insn)
data.sets = sets;
data.n_sets = n_sets_before_autoinc = n_sets;
- for_each_inc_dec (&insn, cselib_record_autoinc_cb, &data);
+ for_each_inc_dec (&PATTERN (insn), cselib_record_autoinc_cb, &data);
n_sets = data.n_sets;
So wouldn't this miss an autoincrement operation embedded in a note,
such as a REG_EQUAL note? My memory is very fuzzy here, but I can't
recall any policy which prohibits an autoincrement addressing mode from
appearing in a REG_EQUAL note. Worse yet, I have vague memories of
embedded side effects actually showing up in REG_EQUAL notes.
But either:
(a) those notes would contain side effects that are also present in the
main pattern, e.g.:
(set (reg Z) (plus (mem (pre_inc X)) (reg Y)))
REG_EQUAL: (plus (mem (pre_inc X)) (const_int Z))
(b) those notes would contain side effects that are not present in the
main pattern.
(b) seems completely invalid to me. REG_EQUAL notes are just a hint
and it's perfectly OK to remove them if they're no longer accurate
(e.g. because a register value used in the note is no longer available).
It's also possible to remove them if the set destination gets combined
with something else. Plus the whole idea of a REG_EQUAL note is that
you could replace the SET_SRC with the note value without changing the
effect of the instruction.
For (a) the current code would end up recording the same side-effect
twice, so looking at just the pattern is likely to be a bug fix.
But (a) is probably invalid too in practice.
The note shows another way to express what appears on the RHS. In
theory the note is supposed to be a simpler form. So in the case of
(a) we might have a PARALLEL in the pattern, but a REG_INC in the note.
I don't see how that'd be terribly helpful though.
I agree (b) is invalid.
Just a guess, but maybe the thing you were thinking of was related to
the old REG_LIBCALL/REG_RETVAL support? Although I only vaguely remember
how that worked now...
I thought it was something in reload's reg_equiv handling that I
stumbled over at some point. The tiny bit I remember was an auto-inc in
the note and something wanting to substitute the note for a use
elsewhere in the insn stream. My recollection was the note had an
auto-inc addressing mode which significantly complicates the validity of
such a transformation.
However, the only thread I can find is one from 2009 between myself and
Ian, but it's not dealing with an autoinc addressing mode in the note.
And at some level it simply doesn't make much sense to have the auto-inc
addressing mode in a REG_EQUAL note. I guess we could declare that
invalid and cope with it if we ever find one. Perhaps a bit of
ENABLE_CHECKING to detect if we ever create such a note?
Jeff
