On Thu, Jun 22, 2017 at 09:25:21AM -0300, Alexandre Oliva wrote:
> On Jun 8, 2017, Segher Boessenkool <seg...@kernel.crashing.org> wrote:
>
> > Would it work to just have "else" instead if this "if"? Or hrm, we'll
> > need to kill the recorded reg_stat value in the last case before this
> > as well?
>
> The patch below (is this what you meant?)
Yes exactly.
> fixes the PR testcase, and the
> new else block doesn't get exercised in an x86_64-linux-gnu bootstrap.
> However, it seems to me that it might very well drop parallel SETs
> without decrementing the sets count, though probably in cases in which
> this won't matter.
>
> How's this? (I haven't run regression tests yet)
Looks a lot better digestable than the previous patch, thanks!
Things should probably be restructured a bit so we keep the sets count
correct, if that is possible?
> When an insn used by combine has multiple SETs, only the non-REG_UNUSED
> set is used: others will end up dropped on the floor.
This isn't exactly true, as I described in a previous email... You can
write something simpler like
If combine drops a REG_UNUSED SET, [...]
> We have to take
> note of the dropped REG_UNUSED SETs, clearing their cached values, so
> that, even if the REGs remain used (e.g. because they were referenced
> in the used SET_SRC), we will not use properties of the latest value
> as if they applied to the earlier one.
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -14087,6 +14087,25 @@ distribute_notes (rtx notes, rtx_insn *from_insn,
> rtx_insn *i3, rtx_insn *i2,
> PUT_REG_NOTE_KIND (note, REG_DEAD);
> place = i3;
> }
> +
> + /* If there were any parallel sets in FROM_INSN other than
> + the one setting IDEST, it must be REG_UNUSED, otherwise
> + we could not have used FROM_INSN in combine. Since this
> + combine attempt succeeded, we know this unused SET was
> + dropped on the floor, because the insn was either deleted
> + or created from a new pattern that does not use its
> + SET_DEST. We must forget whatever we knew about the
> + value that was stored by that SET, since the prior value
> + may still be present in IDEST's src expression or
> + elsewhere, and we do not want to use properties of the
> + dropped value as if they applied to the prior one when
> + simplifying e.g. subsequent combine attempts. */
Similar for this comment. (It has a stray tab btw, before "We").
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr80693.c
> @@ -0,0 +1,26 @@
> +/* { dg-do run } */
> +/* { dg-options "-O -fno-tree-coalesce-vars" } */
> +typedef unsigned char u8;
> +typedef unsigned short u16;
> +typedef unsigned u32;
> +typedef unsigned long u64;
Maybe use "long long"? Less incorrect/misleading on 32-bit targets ;-)
Thanks,
Segher
