This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


[Jeff, sorry for the duplicate, sent the original from an account that
adds disclaimers.]

Jeff Law <law@redhat.com> writes:
> On 08/09/14 04:13, Richard Sandiford wrote:
>> 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.
> 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?

I suppose an assert means that it'd be up to each piece of code that
creates a note to check whether the equivalent value has autoinc addresses.
How about just dropping those REG_EQUAL and REG_EQUIV notes instead,
like we already do for ASM_OPERANDS?

Here I've extended it to all notes with side effects.  The additional
cases are:

    case CLOBBER:
      /* Reject CLOBBER with a non-VOID mode.  These are made by combine.c
	 when some combination can't be done.  If we see one, don't think
	 that we can simplify the expression.  */
      return (GET_MODE (x) != VOIDmode);

    [...snip autoincs...]
    case CALL:
    case UNSPEC_VOLATILE:
      return 1;

    case MEM:
    case ASM_INPUT:
    case ASM_OPERANDS:
      if (MEM_VOLATILE_P (x))
	return 1;

The combine clobbers shouldn't make their way into a note anyway,
since it represents a failed optimisation.  ASM_INPUT is a top-level
rtx rather than a SET_SRC, so isn't important.  Checking for volatile
ASM_OPERANDS is just a subset of the current:

      /* Don't add ASM_OPERAND REG_EQUAL/REG_EQUIV notes.
	 It serves no useful purpose and breaks eliminate_regs.  */
      if (GET_CODE (datum) == ASM_OPERANDS)
	return NULL_RTX;

So the remaining cases are CALL, UNSPEC_VOLATILE and volatile MEMs.
I think UNSPEC_VOLATILE and volatile MEMs really do fall into the same
category as auto-inc/dec.  Const CALLs should be OK in practice,
but I'm not sure why they'd ever need to be treated as having
side effects.  Other CALLs are more dangerous.  In practice the only
interesting notes for calls are (a) that the call address is equal
to some other rtx, which is recorded in the insn that sets the
address register rather than on the call itself and (b) that the result
of the call is equivalent to some non-CALL rtx (e.g. after a libcall).

Tested on x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
	* emit-rtl.c (set_unique_reg_note): Discard notes with side effects.

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	2014-08-26 12:09:28.242659157 +0100
+++ gcc/emit-rtl.c	2014-08-26 12:09:50.694400018 +0100
@@ -5160,6 +5160,14 @@ set_unique_reg_note (rtx insn, enum reg_
 	 It serves no useful purpose and breaks eliminate_regs.  */
       if (GET_CODE (datum) == ASM_OPERANDS)
 	return NULL_RTX;
+
+      /* Notes with side effects are dangerous.  Even if the side-effect
+	 initially mirrors one in PATTERN (INSN), later optimizations
+	 might alter the way that the final register value is calculated
+	 and so move or alter the side-effect in some way.  The note would
+	 then no longer be a valid substitution for SET_SRC.  */
+      if (side_effects_p (datum))
+	return NULL_RTX;
       break;
 
     default:


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]