On 01/23/2018 08:42 PM, Alexandre Oliva wrote:
> These two patches fix PR81611.
>
> The first one improves forwprop so that we avoid adding SSA conflicting
> by forwpropping the iv increment, which may cause both the incremented
> and the original value to be live, even when the iv is copied between
> the PHI node and the increment. We already handled the case in which
> there aren't any such copies.
>
> Alas, this is not enough to address the problem on avr, even though it
> fixes it on e.g. ppc. The reason is that avr rejects var+offset
> addresses, and this prevents the memory access in a post-inc code
> sequence from being adjusted to an address that auto-inc-dec can
> recognize.
>
> The second patch adjusts auto-inc-dec to recognize a code sequence in
> which the original, unincremented pseudo is used in an address after
> it's incremented into another pseudo, and turn that into a post-inc
> address, leaving the copying for subsequent passes to eliminate.
>
> Regstrapped on x86_64-linux-gnu, i686-linux-gnu, ppc64-linux-gnu and
> aarch64-linux-gnu. Ok to install?
>
>
> I'd appreciate suggestions on how to turn the submitted testcase into a
> regression test; I suppose an avr-specific test that requires the
> auto-inc transformation is a possibility, but that feels a bit too
> limited, doesn't it? Thoughts? Thanks in advance,
>
>
[ snip the DOM change which was approved and installed ]
> [PR81611] turn inc-and-use-of-dead-orig into auto-inc
>
> When the addressing modes available on the machine don't allow offsets
> in addresses, odds are that post-increments will be represented in
> trees and RTL as:
>
> y <= x + 1
> ... *(x) ...
> x <= y
>
> so deal with this form so as to create auto-inc addresses that we'd
> otherwise miss.
>
>
> for gcc/ChangeLog
>
> PR rtl-optimization/81611
> * auto-inc-dec.c (attempt_change): Move dead note from
> mem_insn if it's the next use of regno
> (find_address): Take address use of reg holding
> non-incremented value.
> (merge_in_block): Attempt to use a mem insn that is the next
> use of the original regno.
> ---
> gcc/auto-inc-dec.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c
> index d02fa9d081c7..4ffbcf56a456 100644
> --- a/gcc/auto-inc-dec.c
> +++ b/gcc/auto-inc-dec.c
> @@ -508,7 +508,11 @@ attempt_change (rtx new_addr, rtx inc_reg)
> before the memory reference. */
> gcc_assert (mov_insn);
> emit_insn_before (mov_insn, inc_insn.insn);
> - move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0);
> + regno = REGNO (inc_insn.reg0);
> + if (reg_next_use[regno] == mem_insn.insn)
> + move_dead_notes (mov_insn, mem_insn.insn, inc_insn.reg0);
> + else
> + move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0);
>
> regno = REGNO (inc_insn.reg_res);
> reg_next_def[regno] = mov_insn;
> @@ -1370,6 +1385,33 @@ merge_in_block (int max_reg, basic_block bb)
> insn_is_add_or_inc = false;
> }
> }
> +
> + if (ok && insn_is_add_or_inc
> + && inc_insn.reg0 != inc_insn.reg_res)
> + {
> + regno = REGNO (inc_insn.reg0);
> + int luid = DF_INSN_LUID (mem_insn.insn);
> + mem_insn.insn = get_next_ref (regno, bb, reg_next_use);
So I think a comment is warranted right as we enter the TRUE arm.
At that point INC_INSN is an inc/dec. But MEM_INSN is not necessarily a
memory reference. It could be a memory reference, it could be a copy,
it could be something completely different (it's just the next insn that
references the result of the increment). In the case we care about we
want it to be a copy of INC_INSN's REG_RES back to REG0.
ISTM that verifying MEM_INSN is a reg->reg copy (reg_res -> reg0) before
we call get_next_ref for reg0 is advisable and probably good from a
compile-time standpoint by avoiding calls into find_address.
After we call get_next_ref to get the next reference of the source of
the increment, then we're hoping to find a memory reference that uses
REG0. But it's not guaranteed it's a memory reference insn.
I was having an awful time understanding how this code could work from
the comments until I put it under a debugger and got a sense of the
state as we entered that IF block. Then it was much clearer :-)
I believe Georg had other testcases in subsequent comments in the BZ,
but I don't believe they were flagged as regressions. So I think once
you check in your patch we note in the BZ that the original testcase
(and thus the regression) is fixed, but that the later tests are not.
We then remove the regression marker.
If Georg (or anyone else) does the analysis to show those later tests
are also regressions, then we'll add the regression marker back.
Seem reasonable?
Jeff
