[RFC] combine: Improve change_zero_ext, call simplify_set afterwards.

There are several situations that combine.c:change_zero_ext does not
handle well yet.  One of them is

  (and:SI (subreg:SI (zero_extract:DI (reg:DI) ...) ...)
(with const_int operands to "and" and "zero_extract")

=>

  (and:SI (subreg:SI (and:DI (lshiftrt:DI ...)))

with two nested "and"s.  Another one is

  (zero_extract:DI (foo:SI) ...)

which is ignored by change_zero_ext.  Attached are two
experimental patches:

0001-*

  Deal with mode expanding zero_extracts in change_zero_ext.  The
  patch looks good to me, but not sure whether endianness is
  handled properly.  Is the second argument of gen_rtx_SUBREG
  correct?

0002-*

  This is a work in progress with the goal of fixing the first
  problem and similar ones by calling simplify_set after
  change_zero_ext to get rid of the overly complex code.  That
  works fine in principle, but replaces back the (and (lshiftrt
  ...) ...) that change_zero_ext generates back into zero_extract
  form.  Fiddling with simplify_set and make_compound_operation* a
  bit, trying to suppress undoing the transformations that
  change_zero_ext has just done, resulted in the (unfinished)
  patch.

As it's not clear to me whether this is a valid approach I'd
appreciate any advice on the patch or alternative ways of doing
that.

Ciao

Dominik ^_^  ^_^

Hi Dominik,

On Fri, Dec 09, 2016 at 04:23:44PM +0100, Dominik Vogt wrote:
> 0001-*
> 
>   Deal with mode expanding zero_extracts in change_zero_ext.  The
>   patch looks good to me, but not sure whether endianness is
>   handled properly.  Is the second argument of gen_rtx_SUBREG
>   correct?

This we can do; I'll look in detail later.

> 0002-*
> 
>   This is a work in progress with the goal of fixing the first
>   problem and similar ones by calling simplify_set after
>   change_zero_ext to get rid of the overly complex code.  That
>   works fine in principle, but replaces back the (and (lshiftrt
>   ...) ...) that change_zero_ext generates back into zero_extract
>   form.  Fiddling with simplify_set and make_compound_operation* a
>   bit, trying to suppress undoing the transformations that
>   change_zero_ext has just done, resulted in the (unfinished)
>   patch.

This we cannot do: we cannot call simplify* here.  The problem is
combine can reuse the old target reg of an insn it combines but the
simplify* will still see the old nonzero_bits for that register.

The best solution to that is to not reuse registers at all, but that
will have to wait until GCC 8.

> As it's not clear to me whether this is a valid approach I'd
> appreciate any advice on the patch or alternative ways of doing
> that.

Currently I just handle all cases manually in change_zero_ext, but
that is not nice at all for your case.


Segher

On Fri, Dec 09, 2016 at 12:25:04PM -0600, Segher Boessenkool wrote:
> Hi Dominik,
> 
> On Fri, Dec 09, 2016 at 04:23:44PM +0100, Dominik Vogt wrote:
> > 0001-*
> > 
> >   Deal with mode expanding zero_extracts in change_zero_ext.  The
> >   patch looks good to me, but not sure whether endianness is
> >   handled properly.  Is the second argument of gen_rtx_SUBREG
> >   correct?
> 
> This we can do; I'll look in detail later.

Good; thanks.

> > 0002-*
> > 
> >   This is a work in progress with the goal of fixing the first
> >   problem and similar ones by calling simplify_set after
> >   change_zero_ext to get rid of the overly complex code.  That
> >   works fine in principle, but replaces back the (and (lshiftrt
> >   ...) ...) that change_zero_ext generates back into zero_extract
> >   form.  Fiddling with simplify_set and make_compound_operation* a
> >   bit, trying to suppress undoing the transformations that
> >   change_zero_ext has just done, resulted in the (unfinished)
> >   patch.
> 
> This we cannot do: we cannot call simplify* here.  The problem is
> combine can reuse the old target reg of an insn it combines but the
> simplify* will still see the old nonzero_bits for that register.
> 
> The best solution to that is to not reuse registers at all, but that
> will have to wait until GCC 8.
> 
> > As it's not clear to me whether this is a valid approach I'd
> > appreciate any advice on the patch or alternative ways of doing
> > that.
> 
> Currently I just handle all cases manually in change_zero_ext, but
> that is not nice at all for your case.

You mean, inside change_zero_ext just look for for any "(and
(subreg (zero_extract ...)))" where the zero_extract gets replaced
later and the two and masks can be merged into one, and do that
manually?  I'll try that.

Ciao

Dominik ^_^  ^_^

On Fri, Dec 09, 2016 at 08:32:01PM +0100, Dominik Vogt wrote:
> > >   This is a work in progress with the goal of fixing the first
> > >   problem and similar ones by calling simplify_set after
> > >   change_zero_ext to get rid of the overly complex code.  That
> > >   works fine in principle, but replaces back the (and (lshiftrt
> > >   ...) ...) that change_zero_ext generates back into zero_extract
> > >   form.  Fiddling with simplify_set and make_compound_operation* a
> > >   bit, trying to suppress undoing the transformations that
> > >   change_zero_ext has just done, resulted in the (unfinished)
> > >   patch.
> > 
> > This we cannot do: we cannot call simplify* here.  The problem is
> > combine can reuse the old target reg of an insn it combines but the
> > simplify* will still see the old nonzero_bits for that register.
> > 
> > The best solution to that is to not reuse registers at all, but that
> > will have to wait until GCC 8.
> > 
> > > As it's not clear to me whether this is a valid approach I'd
> > > appreciate any advice on the patch or alternative ways of doing
> > > that.
> > 
> > Currently I just handle all cases manually in change_zero_ext, but
> > that is not nice at all for your case.
> 
> You mean, inside change_zero_ext just look for for any "(and
> (subreg (zero_extract ...)))" where the zero_extract gets replaced
> later and the two and masks can be merged into one, and do that
> manually?  I'll try that.

You can also code those patterns (in your MD) as the zero_extract
form, if that works nicer?  This is for RNSBG, right, so you also
need to handles RXSBG and ROSBG, maybe you can do those in the
same pattern?


Segher

On Fri, Dec 09, 2016 at 04:23:44PM +0100, Dominik Vogt wrote:
> 0001-*
> 
>   Deal with mode expanding zero_extracts in change_zero_ext.  The
>   patch looks good to me, but not sure whether endianness is
>   handled properly.  Is the second argument of gen_rtx_SUBREG
>   correct?

> >From 600ed3dadd5bc2568ab53be8466686abaf27ff3f Mon Sep 17 00:00:00 2001
> From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> Date: Fri, 9 Dec 2016 02:48:30 +0100
> Subject: [PATCH 1/2] combine: Handle mode expanding zero_extracts in
>  change_zero_ext.
> 
> Example:
> 
>   (zero_extract:DI (reg:SI)
>                    (const_int 24)
>                    (const_int 0))
> 
> -->
> 
>   (and:DI (subreg:DI (lshiftrt:SI (reg:SI) (const_int 8))
>                      0)
>           (const_int 16777215))
> ---
>  gcc/combine.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/combine.c b/gcc/combine.c
> index b429453..e14a08f 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -11237,18 +11237,24 @@ change_zero_ext (rtx pat)
>        if (GET_CODE (x) == ZERO_EXTRACT
>  	  && CONST_INT_P (XEXP (x, 1))
>  	  && CONST_INT_P (XEXP (x, 2))
> -	  && GET_MODE (XEXP (x, 0)) == mode)
> +	  && (GET_MODE (XEXP (x, 0)) == mode
> +	      || GET_MODE_PRECISION (GET_MODE (XEXP (x, 0)))
> +		 < GET_MODE_PRECISION (mode)))

If you make this <= you can collapse the two cases into one.

>  	{
> +	  machine_mode inner_mode = GET_MODE (XEXP (x, 0));
> +
>  	  size = INTVAL (XEXP (x, 1));
>  
>  	  int start = INTVAL (XEXP (x, 2));
>  	  if (BITS_BIG_ENDIAN)
> -	    start = GET_MODE_PRECISION (mode) - size - start;
> +	    start = GET_MODE_PRECISION (inner_mode) - size - start;
>  
>  	  if (start)
> -	    x = gen_rtx_LSHIFTRT (mode, XEXP (x, 0), GEN_INT (start));
> +	    x = gen_rtx_LSHIFTRT (inner_mode, XEXP (x, 0), GEN_INT (start));
>  	  else
>  	    x = XEXP (x, 0);
> +	  if (mode != inner_mode)
> +	    x = gen_rtx_SUBREG (mode, x, 0);

gen_lowpart_SUBREG instead?  It's easier to read, and code a little
further down does the same thing.

Okay for trunk with those changes (did you bootstrap+regcheck this
already?)  Thanks,


Segher

On Sat, Dec 10, 2016 at 10:37:38AM -0600, Segher Boessenkool wrote:
> Okay for trunk with those changes (did you bootstrap+regcheck this
> already?)  Thanks,

Oh, and a changelog :-)


Segher

On Sat, Dec 10, 2016 at 10:37:38AM -0600, Segher Boessenkool wrote:
> On Fri, Dec 09, 2016 at 04:23:44PM +0100, Dominik Vogt wrote:
> > 0001-*
> > 
> >   Deal with mode expanding zero_extracts in change_zero_ext.  The
> >   patch looks good to me, but not sure whether endianness is
> >   handled properly.  Is the second argument of gen_rtx_SUBREG
> >   correct?
> 
> > >From 600ed3dadd5bc2568ab53be8466686abaf27ff3f Mon Sep 17 00:00:00 2001
> > From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> > Date: Fri, 9 Dec 2016 02:48:30 +0100
> > Subject: [PATCH 1/2] combine: Handle mode expanding zero_extracts in
> >  change_zero_ext.
> > 
> > Example:
> > 
> >   (zero_extract:DI (reg:SI)
> >                    (const_int 24)
> >                    (const_int 0))
> > 
> > -->
> > 
> >   (and:DI (subreg:DI (lshiftrt:SI (reg:SI) (const_int 8))
> >                      0)
> >           (const_int 16777215))
> > ---
> >  gcc/combine.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gcc/combine.c b/gcc/combine.c
> > index b429453..e14a08f 100644
> > --- a/gcc/combine.c
> > +++ b/gcc/combine.c
> > @@ -11237,18 +11237,24 @@ change_zero_ext (rtx pat)
> >        if (GET_CODE (x) == ZERO_EXTRACT
> >  	  && CONST_INT_P (XEXP (x, 1))
> >  	  && CONST_INT_P (XEXP (x, 2))
> > -	  && GET_MODE (XEXP (x, 0)) == mode)
> > +	  && (GET_MODE (XEXP (x, 0)) == mode
> > +	      || GET_MODE_PRECISION (GET_MODE (XEXP (x, 0)))
> > +		 < GET_MODE_PRECISION (mode)))
> 
> If you make this <= you can collapse the two cases into one.
> 
> >  	{
> > +	  machine_mode inner_mode = GET_MODE (XEXP (x, 0));
> > +
> >  	  size = INTVAL (XEXP (x, 1));
> >  
> >  	  int start = INTVAL (XEXP (x, 2));
> >  	  if (BITS_BIG_ENDIAN)
> > -	    start = GET_MODE_PRECISION (mode) - size - start;
> > +	    start = GET_MODE_PRECISION (inner_mode) - size - start;
> >  
> >  	  if (start)
> > -	    x = gen_rtx_LSHIFTRT (mode, XEXP (x, 0), GEN_INT (start));
> > +	    x = gen_rtx_LSHIFTRT (inner_mode, XEXP (x, 0), GEN_INT (start));
> >  	  else
> >  	    x = XEXP (x, 0);
> > +	  if (mode != inner_mode)
> > +	    x = gen_rtx_SUBREG (mode, x, 0);
> 
> gen_lowpart_SUBREG instead?  It's easier to read, and code a little
> further down does the same thing.

Thanks for taking a look.

> Okay for trunk with those changes (did you bootstrap+regcheck this
> already?)

The patch hasn't got any real testing yet.  I'll try to do that
today or tomorrow.

Ciao

Dominik ^_^  ^_^