| Message ID | 8761i97ieu.fsf@googlemail.com |
|---|---|
| State | New |
| Headers | show |
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. Similarly for other places where you're passing down the pattern rather than the insn itself. Thoughts/comments? Does this trigger any disturbing memories for anyone else? jeff
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
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? Jeff
Index: gcc/rtl.h =================================================================== --- gcc/rtl.h 2014-08-03 11:25:31.042163078 +0100 +++ gcc/rtl.h 2014-08-03 11:25:31.324165866 +0100 @@ -2293,9 +2293,8 @@ extern int for_each_rtx (rtx *, rtx_func within MEM that sets DEST to SRC + SRCOFF, or SRC if SRCOFF is NULL. The callback is passed the same opaque ARG passed to for_each_inc_dec. Return zero to continue looking for other - autoinc operations, -1 to skip OP's operands, and any other value - to interrupt the traversal and return that value to the caller of - for_each_inc_dec. */ + autoinc operations or any other value to interrupt the traversal and + return that value to the caller of for_each_inc_dec. */ typedef int (*for_each_inc_dec_fn) (rtx mem, rtx op, rtx dest, rtx src, rtx srcoff, void *arg); extern int for_each_inc_dec (rtx *, for_each_inc_dec_fn, void *arg); Index: gcc/cselib.c =================================================================== --- gcc/cselib.c 2014-08-03 11:25:09.536950464 +0100 +++ gcc/cselib.c 2014-08-03 11:25:31.323165856 +0100 @@ -2464,7 +2464,7 @@ cselib_record_autoinc_cb (rtx mem ATTRIB data->n_sets++; - return -1; + return 0; } /* Record the effects of any sets and autoincs in INSN. */ @@ -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; /* Look up the values that are read. Do this before invalidating the Index: gcc/dse.c =================================================================== --- gcc/dse.c 2014-08-03 11:25:25.192105241 +0100 +++ gcc/dse.c 2014-08-03 11:25:31.323165856 +0100 @@ -895,7 +895,7 @@ emit_inc_dec_insn_before (rtx mem ATTRIB emit_insn_before (new_insn, insn); - return -1; + return 0; } /* Before we delete INSN_INFO->INSN, make sure that the auto inc/dec, if it @@ -908,7 +908,8 @@ check_for_inc_dec_1 (insn_info_t insn_in rtx insn = insn_info->insn; rtx note = find_reg_note (insn, REG_INC, NULL_RTX); if (note) - return for_each_inc_dec (&insn, emit_inc_dec_insn_before, insn_info) == 0; + return for_each_inc_dec (&PATTERN (insn), emit_inc_dec_insn_before, + insn_info) == 0; return true; } @@ -927,7 +928,8 @@ check_for_inc_dec (rtx insn) insn_info.fixed_regs_live = NULL; note = find_reg_note (insn, REG_INC, NULL_RTX); if (note) - return for_each_inc_dec (&insn, emit_inc_dec_insn_before, &insn_info) == 0; + return for_each_inc_dec (&PATTERN (insn), emit_inc_dec_insn_before, + &insn_info) == 0; return true; } Index: gcc/rtlanal.c =================================================================== --- gcc/rtlanal.c 2014-08-03 11:25:31.043163088 +0100 +++ gcc/rtlanal.c 2014-08-03 11:25:31.325165876 +0100 @@ -3105,49 +3105,32 @@ for_each_rtx (rtx *x, rtx_function f, vo -/* Data structure that holds the internal state communicated between - for_each_inc_dec, for_each_inc_dec_find_mem and - for_each_inc_dec_find_inc_dec. */ - -struct for_each_inc_dec_ops { - /* The function to be called for each autoinc operation found. */ - for_each_inc_dec_fn fn; - /* The opaque argument to be passed to it. */ - void *arg; - /* The MEM we're visiting, if any. */ - rtx mem; -}; - -static int for_each_inc_dec_find_mem (rtx *r, void *d); - -/* Find PRE/POST-INC/DEC/MODIFY operations within *R, extract the - operands of the equivalent add insn and pass the result to the - operator specified by *D. */ +/* MEM has a PRE/POST-INC/DEC/MODIFY address X. Extract the operands of + the equivalent add insn and pass the result to FN, using DATA as the + final argument. */ static int -for_each_inc_dec_find_inc_dec (rtx *r, void *d) +for_each_inc_dec_find_inc_dec (rtx mem, for_each_inc_dec_fn fn, void *data) { - rtx x = *r; - struct for_each_inc_dec_ops *data = (struct for_each_inc_dec_ops *)d; - + rtx x = XEXP (mem, 0); switch (GET_CODE (x)) { case PRE_INC: case POST_INC: { - int size = GET_MODE_SIZE (GET_MODE (data->mem)); + int size = GET_MODE_SIZE (GET_MODE (mem)); rtx r1 = XEXP (x, 0); rtx c = gen_int_mode (size, GET_MODE (r1)); - return data->fn (data->mem, x, r1, r1, c, data->arg); + return fn (mem, x, r1, r1, c, data); } case PRE_DEC: case POST_DEC: { - int size = GET_MODE_SIZE (GET_MODE (data->mem)); + int size = GET_MODE_SIZE (GET_MODE (mem)); rtx r1 = XEXP (x, 0); rtx c = gen_int_mode (-size, GET_MODE (r1)); - return data->fn (data->mem, x, r1, r1, c, data->arg); + return fn (mem, x, r1, r1, c, data); } case PRE_MODIFY: @@ -3155,69 +3138,43 @@ for_each_inc_dec_find_inc_dec (rtx *r, v { rtx r1 = XEXP (x, 0); rtx add = XEXP (x, 1); - return data->fn (data->mem, x, r1, add, NULL, data->arg); - } - - case MEM: - { - rtx save = data->mem; - int ret = for_each_inc_dec_find_mem (r, d); - data->mem = save; - return ret; + return fn (mem, x, r1, add, NULL, data); } default: - return 0; - } -} - -/* If *R is a MEM, find PRE/POST-INC/DEC/MODIFY operations within its - address, extract the operands of the equivalent add insn and pass - the result to the operator specified by *D. */ - -static int -for_each_inc_dec_find_mem (rtx *r, void *d) -{ - rtx x = *r; - if (x != NULL_RTX && MEM_P (x)) - { - struct for_each_inc_dec_ops *data = (struct for_each_inc_dec_ops *) d; - int result; - - data->mem = x; - - result = for_each_rtx (&XEXP (x, 0), for_each_inc_dec_find_inc_dec, - data); - if (result) - return result; - - return -1; + gcc_unreachable (); } - return 0; } -/* Traverse *X looking for MEMs, and for autoinc operations within - them. For each such autoinc operation found, call FN, passing it +/* Traverse *LOC looking for MEMs that have autoinc addresses. + For each such autoinc operation found, call FN, passing it the innermost enclosing MEM, the operation itself, the RTX modified by the operation, two RTXs (the second may be NULL) that, once added, represent the value to be held by the modified RTX - afterwards, and ARG. FN is to return -1 to skip looking for other - autoinc operations within the visited operation, 0 to continue the - traversal, or any other value to have it returned to the caller of + afterwards, and DATA. FN is to return 0 to continue the + traversal or any other value to have it returned to the caller of for_each_inc_dec. */ int -for_each_inc_dec (rtx *x, +for_each_inc_dec (rtx *loc, for_each_inc_dec_fn fn, - void *arg) + void *data) { - struct for_each_inc_dec_ops data; - - data.fn = fn; - data.arg = arg; - data.mem = NULL; - - return for_each_rtx (x, for_each_inc_dec_find_mem, &data); + subrtx_var_iterator::array_type array; + FOR_EACH_SUBRTX_VAR (iter, array, *loc, NONCONST) + { + rtx mem = *iter; + if (mem + && MEM_P (mem) + && GET_RTX_CLASS (GET_CODE (XEXP (mem, 0))) == RTX_AUTOINC) + { + int res = for_each_inc_dec_find_inc_dec (mem, fn, data); + if (res != 0) + return res; + iter.skip_subrtxes (); + } + } + return 0; }