simplify-rtx: Two easy pieces.

Message ID 008901d6467a$36155350$a23ff9f0$@nextmovesoftware.com
State New
Headers show
Series
  • simplify-rtx: Two easy pieces.
Related show

Commit Message

Roger Sayle June 19, 2020, 8:42 p.m.
My recent patch to add scalar integer simplification unit tests to simplify_rtx_c_tests
identified two "trivial" corner cases that could be improved in simplify-rtx.c.
I don't believe that either case can currently be triggered from GCC current
front-end/back-end combinations, but hopefully the reviewer agrees these
changes are simple/safe enough.

Although it makes no sense to ever see a BImode ROTATE, the current ordering
of transformations in simplify_binary_operation_1 converts (rotate:bi (reg:bi) 0) to
(rotatert:bi (reg:bi) 1), which then doesn't get simplified away.  Rather than teach
the middle-end that any hypothetical ROTATE or ROTATERT of a BImode value is a
no-op, a more realistic invariant is that any rotate by const0_rtx is already canonical.

Optimizing "parity of parity" matches the tree constant folding transformation pending
review.  Alas, the only mentions of PARITY in GCC's official backend machine descriptions
are in expanders, so the middle-end's RTL optimizers never see a PARITY to simplify.
A test can be added to test_scalar_int_ops once that patch is reviewed/approved.

This patch has been tested with "make bootstrap" and "make -k check"
on x86_64-pc-linux-gnu with no regressions.


2020-06-19  Roger Sayle  <roger@nextmovesoftware.com>

        * simplify-rtx.c (simplify_unary_operation_1): Simplify
        (parity (parity x)) as (parity x), i.e. PARITY is idempotent.
        (simplify_binary_operation_1): Don't canonicalize rotations
        by zero bits, these get simplified away.


Thanks in advance,
Roger
--
Roger Sayle
NextMove Software
Cambridge, UK

Comments

Segher Boessenkool June 20, 2020, 11:03 a.m. | #1
Hi!

On Fri, Jun 19, 2020 at 09:42:54PM +0100, Roger Sayle wrote:
> My recent patch to add scalar integer simplification unit tests to simplify_rtx_c_tests

> identified two "trivial" corner cases that could be improved in simplify-rtx.c.


These two things are independent changes and should be independent
patches.

> Although it makes no sense to ever see a BImode ROTATE, the current ordering

> of transformations in simplify_binary_operation_1 converts (rotate:bi (reg:bi) 0) to

> (rotatert:bi (reg:bi) 1), which then doesn't get simplified away.  Rather than teach

> the middle-end that any hypothetical ROTATE or ROTATERT of a BImode value is a

> no-op, a more realistic invariant is that any rotate by const0_rtx is already canonical.


I don't know of a rotate of BImode is defined at all, but that is beside
the point, the patch has nothing to do with BImode.

A rotate by constant 0 should be simplified to just its argument, and
that should happen *before* where your patch makes changes.  What goes
wrong there?

> Optimizing "parity of parity" matches the tree constant folding transformation pending

> review.  Alas, the only mentions of PARITY in GCC's official backend machine descriptions

> are in expanders, so the middle-end's RTL optimizers never see a PARITY to simplify.


This part is approved for trunk.  Thanks!

> A test can be added to test_scalar_int_ops once that patch is reviewed/approved.


If you insist.  I find this futile busy-work, now and in the future :-/


Segher
Roger Sayle June 20, 2020, 4:25 p.m. | #2
Hi Segher,
It's great to hear from you again.   It's been a while.

>On Fri, Jun 19, 2020 at 09:42:54PM +0100, Roger Sayle wrote:

>> My recent patch to add scalar integer simplification unit tests to 

>> simplify_rtx_c_tests identified two "trivial" corner cases that could be

improved in simplify-rtx.c.
>

> These two things are independent changes and should be independent

patches.

I'm about to ask you (very nicely) to please commit these changes for me,
so many thanks in advance if you're going to be making twice the effort.

>> Although it makes no sense to ever see a BImode ROTATE, the current 

>> ordering of transformations in simplify_binary_operation_1 converts 

>> (rotate:bi (reg:bi) 0) to (rotatert:bi (reg:bi) 1), which then doesn't 

>> get simplified away.  Rather than teach the middle-end that any 

>> hypothetical ROTATE or ROTATERT of a BImode value is a no-op, a more

realistic invariant is that any rotate by const0_rtx is already canonical.
>

> I don't know of a rotate of BImode is defined at all, but that is beside

the point, the patch has nothing to do with BImode.
> A rotate by constant 0 should be simplified to just its argument, and that

should happen *before* where your patch makes changes.  What goes wrong
there?

This approach becomes clearer from seeing the larger context of this change:

    case ROTATERT:
    case ROTATE:
      /* Canonicalize rotates by constant amount.  If op1 is bitsize / 2,
         prefer left rotation, if op1 is from bitsize / 2 + 1 to
         bitsize - 1, use other direction of rotate with 1 .. bitsize / 2 -
1
         amount instead.  */
#if defined(HAVE_rotate) && defined(HAVE_rotatert)
      if (CONST_INT_P (trueop1)
          && IN_RANGE (INTVAL (trueop1),
                       GET_MODE_UNIT_PRECISION (mode) / 2 + (code ==
ROTATE),
                       GET_MODE_UNIT_PRECISION (mode) - 1))
        {
          int new_amount = GET_MODE_UNIT_PRECISION (mode) - INTVAL
(trueop1);
          rtx new_amount_rtx = gen_int_shift_amount (mode, new_amount);
          return simplify_gen_binary (code == ROTATE ? ROTATERT : ROTATE,
                                      mode, op0, new_amount_rtx);
        }
#endif
      /* FALLTHRU */
    case ASHIFTRT:
      if (trueop1 == CONST0_RTX (mode))
        return op0;

Hence the handling of rotates and shifts by zero is the very next
optimization, after the fall-thru.
One option would be to duplicate this const0 optimization before the rotate
canonicalization, then have
it tested a second time later (perhaps twice sequentially if the backend
doesn't support both rotates)
or add a goto.  I believed the simplest fix is to correct the condition for
canonicalizing which shouldn't
fire when op1 is const0_rtx.

You'll also notice that by placing "trueop1 != CONST0_RTX (mode)" condition
as the very last clause
in the test, this creates a jump threading opportunity for the compiler.
[p.s. Many thanks again
for your help back in 2002/2003 when I originally added jump threading to
GCC].

>> Optimizing "parity of parity" matches the tree constant folding 

>> transformation pending review.  Alas, the only mentions of PARITY in 

>> GCC's official backend machine descriptions are in expanders, so the

>> middle-end's RTL optimizers never see a PARITY to simplify.

>

> This part is approved for trunk.  Thanks!


Thanks to you too.  Alas, my credentials from the CVS days of GCC almost
certainly don't
work any more (in git), so I'm now reliant on the generosity of others to
push changes
into the tree.  I promise that if I make a habit of contributing regularly
again, I'll learn 
the new ways of doing things, but it's a little awkward after being a
maintainer.

>> A test can be added to test_scalar_int_ops once that patch is

reviewed/approved.
> If you insist.  I find this futile busy-work, now and in the future :-/

I'm just following Richard Sandiford suggestions, but I will concede that
these internal
tests did spot the (admittedly minor) logic issue above.

Please let me know what you think.  And many thanks again.

Cheers,
Roger
--
Hans-Peter Nilsson June 20, 2020, 7:29 p.m. | #3
Hi!  Good to see you "back"!

On Sat, 20 Jun 2020, Roger Sayle wrote:
> Thanks to you too.  Alas, my credentials from the CVS days of GCC almost

> certainly don't

> work any more (in git),


My guess is that your credentials are fine (possibly modulo FSF
assignment issues) if it wasn't for the ssh key for your account
being unacceptably short and/or DSA.  I'd suggest you generate a
new larger-than-X-bits RSA key (4096 should be good) and ask
overseers@ to install it for you.

brgds, H-P
Joseph Myers June 22, 2020, 8:51 p.m. | #4
On Sat, 20 Jun 2020, Hans-Peter Nilsson wrote:

> Hi!  Good to see you "back"!

> 

> On Sat, 20 Jun 2020, Roger Sayle wrote:

> > Thanks to you too.  Alas, my credentials from the CVS days of GCC almost

> > certainly don't

> > work any more (in git),

> 

> My guess is that your credentials are fine (possibly modulo FSF

> assignment issues) if it wasn't for the ssh key for your account

> being unacceptably short and/or DSA.  I'd suggest you generate a

> new larger-than-X-bits RSA key (4096 should be good) and ask

> overseers@ to install it for you.


In this particular case, the reason a new key is probably needed is that 
the one currently installed for Roger's account is in SSH1 format (and SSH 
protocol version 1 is long obsolete, and I don't think SSH servers accept 
SSH1 format keys in authorized_keys for SSH protocol 2).

-- 
Joseph S. Myers
joseph@codesourcery.com
Segher Boessenkool June 22, 2020, 10:59 p.m. | #5
Hi!

On Sat, Jun 20, 2020 at 05:25:23PM +0100, Roger Sayle wrote:
> It's great to hear from you again.   It's been a while.


And from you!  Yes, wow, it is 2020 now, how did that happen!

> >On Fri, Jun 19, 2020 at 09:42:54PM +0100, Roger Sayle wrote:

> >> My recent patch to add scalar integer simplification unit tests to 

> >> simplify_rtx_c_tests identified two "trivial" corner cases that could be

> improved in simplify-rtx.c.

> >

> > These two things are independent changes and should be independent

> patches.

> 

> I'm about to ask you (very nicely) to please commit these changes for me,

> so many thanks in advance if you're going to be making twice the effort.


Yes, I'll do it (tomorrow, tired now).

> >> Although it makes no sense to ever see a BImode ROTATE, the current 

> >> ordering of transformations in simplify_binary_operation_1 converts 

> >> (rotate:bi (reg:bi) 0) to (rotatert:bi (reg:bi) 1), which then doesn't 

> >> get simplified away.  Rather than teach the middle-end that any 

> >> hypothetical ROTATE or ROTATERT of a BImode value is a no-op, a more

> realistic invariant is that any rotate by const0_rtx is already canonical.

> >

> > I don't know of a rotate of BImode is defined at all, but that is beside

> the point, the patch has nothing to do with BImode.

> > A rotate by constant 0 should be simplified to just its argument, and that

> should happen *before* where your patch makes changes.  What goes wrong

> there?

> 

> This approach becomes clearer from seeing the larger context of this change:

> 

>     case ROTATERT:

>     case ROTATE:

>       /* Canonicalize rotates by constant amount.  If op1 is bitsize / 2,

>          prefer left rotation, if op1 is from bitsize / 2 + 1 to

>          bitsize - 1, use other direction of rotate with 1 .. bitsize / 2 -

> 1

>          amount instead.  */

> #if defined(HAVE_rotate) && defined(HAVE_rotatert)

>       if (CONST_INT_P (trueop1)

>           && IN_RANGE (INTVAL (trueop1),

>                        GET_MODE_UNIT_PRECISION (mode) / 2 + (code ==

> ROTATE),

>                        GET_MODE_UNIT_PRECISION (mode) - 1))

>         {

>           int new_amount = GET_MODE_UNIT_PRECISION (mode) - INTVAL

> (trueop1);

>           rtx new_amount_rtx = gen_int_shift_amount (mode, new_amount);

>           return simplify_gen_binary (code == ROTATE ? ROTATERT : ROTATE,

>                                       mode, op0, new_amount_rtx);

>         }

> #endif

>       /* FALLTHRU */

>     case ASHIFTRT:

>       if (trueop1 == CONST0_RTX (mode))

>         return op0;

> 

> Hence the handling of rotates and shifts by zero is the very next

> optimization, after the fall-thru.


Fall throughs are evil.  I'm not kidding very much here :-)

> One option would be to duplicate this const0 optimization before the rotate

> canonicalization, then have

> it tested a second time later (perhaps twice sequentially if the backend

> doesn't support both rotates)

> or add a goto.  I believed the simplest fix is to correct the condition for

> canonicalizing which shouldn't

> fire when op1 is const0_rtx.


That is the smallest change to make, certainly.  But it doesn't result
in the simplest (i.e. easiest to read) code.

I'll see what I can do.  Probably it turns out to be involved enough
that I'll just do your change, but :-)

> > This part is approved for trunk.  Thanks!

> 

> Thanks to you too.  Alas, my credentials from the CVS days of GCC almost

> certainly don't

> work any more (in git), so I'm now reliant on the generosity of others to

> push changes

> into the tree.


It still accepts my 2000-era RSA key, apparently (but not DSA any
more :-( )

> I promise that if I make a habit of contributing regularly

> again, I'll learn 

> the new ways of doing things, but it's a little awkward after being a

> maintainer.


Not too much changed!  Well, maybe all the details did ;-)


Segher

Patch

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 28c2dc6..b856b02 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -1391,6 +1391,10 @@  simplify_unary_operation_1 (enum rtx_code code, machine_mode mode, rtx op)
 				       GET_MODE (XEXP (op, 0)));
 	  break;
 
+	case PARITY:
+	  /* (parity (parity x)) -> parity (x).  */
+	  return op;
+
 	default:
 	  break;
 	}
@@ -3649,7 +3653,8 @@  simplify_binary_operation_1 (enum rtx_code code, machine_mode mode,
       if (CONST_INT_P (trueop1)
 	  && IN_RANGE (INTVAL (trueop1),
 		       GET_MODE_UNIT_PRECISION (mode) / 2 + (code == ROTATE),
-		       GET_MODE_UNIT_PRECISION (mode) - 1))
+		       GET_MODE_UNIT_PRECISION (mode) - 1)
+	  && trueop1 != CONST0_RTX (mode))
 	{
 	  int new_amount = GET_MODE_UNIT_PRECISION (mode) - INTVAL (trueop1);
 	  rtx new_amount_rtx = gen_int_shift_amount (mode, new_amount);