simplify-rtx: Two easy pieces.

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

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

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
--

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

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).

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