Re: [PR80693] drop value of parallel SETs dropped by combine

[Segher Boessenkool <segher at kernel dot crashing dot org>]

On Thu, Jun 22, 2017 at 09:25:21AM -0300, Alexandre Oliva wrote:
> On Jun  8, 2017, Segher Boessenkool <segher@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