Re: [PATCH 40/50] rtlanal.c:for_each_inc_dec

[Richard Sandiford <rdsandiford at googlemail dot com>]

Jeff Law <law@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.

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...

Thanks,
Richard