[Bug target/100056] New: [9/10/11 Regression]

[Bug target/100056] New: [9/10/11 Regression]

[luc.vanoostenryck at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

            Bug ID: 100056
           Summary: [9/10/11 Regression]
           Product: gcc
           Version: 9.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: luc.vanoostenryck at gmail dot com
  Target Milestone: ---
            Target: aarch64

Created attachment 50573
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50573&action=edit
or-shift vs. [us]bfiz

On arm64, the following code:
    unsigned or_shift(unsigned char i)
    {
        return i | (i << 11);
    }

translate to the following assembly:
    or_shift:
        and     w1, w0, 255
        ubfiz   w0, w0, 11, 8
        orr     w0, w0, w1
        ret

where the ubfiz instruction is a bit weird since the code
matches directly what was generated in gcc 8.x and before:
    or_shift:
        and     w0, w0, 255
        orr     w0, w0, w0, lsl 11
        ret

Same with a signed argument (see https://gcc.godbolt.org/z/af4zffMYa ).

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-04-13
   Target Milestone|---                         |9.4
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r9-2064-gc4c5ad1d6d1e1e1fe7a1c2b3bb097cc269dc7306

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
           Priority|P3                          |P2

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, the change is that previously there was just one significant combination,
Trying 7 -> 8:
    7: r96:SI=r94:SI<<0xb
    8: r95:SI=r96:SI|r94:SI
      REG_DEAD r96:SI
      REG_DEAD r94:SI
Successfully matched this instruction:
(set (reg:SI 95)
    (ior:SI (ashift:SI (reg/v:SI 94 [ i ])
            (const_int 11 [0xb]))
        (reg/v:SI 94 [ i ])))
With the addition of 2 to 2 combination,
Trying 2 -> 7:
    2: r94:SI=zero_extend(x0:QI)
      REG_DEAD x0:QI
    7: r96:SI=r94:SI<<0xb
is replaced with:
(set (reg/v:SI 94 [ i ])
    (zero_extend:SI (reg:QI 0 x0 [ i ])))
and
(set (reg:SI 96)
    (and:SI (ashift:SI (reg:SI 0 x0 [ i ])
            (const_int 11 [0xb]))
        (const_int 522240 [0x7f800])))
and therefore the combination that was successful earlier no longer triggers.
So, I think it would be helpful to have a combiner splitter that would split
Trying 7, 2 -> 8:
    7: r96:SI=x0:SI<<0xb&0x7f800
      REG_DEAD x0:QI
    2: r94:SI=zero_extend(x0:QI)
    8: r95:SI=r96:SI|r94:SI
the zero_extend and
(set (reg:SI 95)
    (ior:SI (ashift:SI (reg/v:SI 94 [ i ])
            (const_int 11 [0xb]))
        (reg/v:SI 94 [ i ])))

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50583
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50583&action=edit
gcc11-pr100056.patch

Untested fix.

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[luc.vanoostenryck at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #4 from Luc Van Oostenryck <luc.vanoostenryck at gmail dot com> ---
(In reply to Jakub Jelinek from comment #3)
> Created attachment 50583 [details]
> gcc11-pr100056.patch
> 
> Untested fix.

Mmmm, that's working fine for the cases I had but not in
more general cases. I think that the constraint on the AND
may be too tight. For example, changing things slightly to
have a smaller mask:
    int or_lsl_u3(unsigned i) {
        i &= 7;
        return i | (i << 11);
    }

still gives:
    or_lsl_u3:
        and     w1, w0, 7
        ubfiz   w0, w0, 11, 3
        orr     w0, w0, w1
        ret

while GCC8 gave the expected:
    or_lsl_u3:
        and     w0, w0, 7
        orr     w0, w0, w0, lsl 11
        ret

In fact, I would tend to think that the AND part should be
removed from your split pattern (some kind of zero-extension
seems to be needed to reproduce the problem but that's all).

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[luc.vanoostenryck at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #5 from Luc Van Oostenryck <luc.vanoostenryck at gmail dot com> ---
Created attachment 50584
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50584&action=edit
updated test cases

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[luc.vanoostenryck at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #6 from Luc Van Oostenryck <luc.vanoostenryck at gmail dot com> ---
(In reply to Jakub Jelinek from comment #3)
> Created attachment 50583 [details]
> gcc11-pr100056.patch
> 
> Untested fix.

OTOH, for the signed case things seems to be OK unless the
sign extension is one of the register sizes (8, 16 & 32).

See the updated testcases in attachment.

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[luc.vanoostenryck at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #7 from Luc Van Oostenryck <luc.vanoostenryck at gmail dot com> ---
Created attachment 50585
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50585&action=edit
newer testcases (with 32 -> 64-bit extensions)

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50586
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50586&action=edit
gcc11-pr100056.patch

Updated patch.

[Bug target/100056] [9/10/11 Regression] orr + lsl vs. [us]bfiz

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:39d23b7960e4efb11bbe1eff056ae9da0884c539

commit r11-8188-g39d23b7960e4efb11bbe1eff056ae9da0884c539
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Apr 15 10:45:09 2021 +0200

    aarch64: Fix several *<LOGICAL:optab>_ashl<mode>3 related regressions
[PR100056]

    Before combiner added 2 to 2 combinations, the following testcase functions
    have been all compiled into 2 instructions, zero/sign extensions or and
    followed by orr with lsl, e.g. for the first function
    Trying 7 -> 8:
        7: r96:SI=r94:SI<<0xb
        8: r95:SI=r96:SI|r94:SI
          REG_DEAD r96:SI
          REG_DEAD r94:SI
    Successfully matched this instruction:
    (set (reg:SI 95)
        (ior:SI (ashift:SI (reg/v:SI 94 [ i ])
                (const_int 11 [0xb]))
            (reg/v:SI 94 [ i ])))
    is the important successful try_combine and so we end up with
            and     w0, w0, 255
            orr     w0, w0, w0, lsl 11
    in the body.
    With 2 to 2 combination, before that can trigger, another successful
    combination:
    Trying 2 -> 7:
        2: r94:SI=zero_extend(x0:QI)
          REG_DEAD x0:QI
        7: r96:SI=r94:SI<<0xb
    is replaced with:
    (set (reg/v:SI 94 [ i ])
        (zero_extend:SI (reg:QI 0 x0 [ i ])))
    and
    (set (reg:SI 96)
        (and:SI (ashift:SI (reg:SI 0 x0 [ i ])
                (const_int 11 [0xb]))
            (const_int 522240 [0x7f800])))
    and in the end results in 3 instructions in the body:
            and     w1, w0, 255
            ubfiz   w0, w0, 11, 8
            orr     w0, w0, w1
    The following combine splitters help undo that when combiner tries to
    combine 3 instructions - the zero/sign extend or and, the other insn
    from the 2 to 2 combination ([us]bfiz) and the logical op, the CPUs
    don't have an insn to do everything in one op, but we can split it
    back into the zero/sign extend or and followed by logical with lsl.

    2021-04-15  Jakub Jelinek  <jakub@redhat.com>

            PR target/100056
            * config/aarch64/aarch64.md
(*<LOGICAL:optab>_<SHIFT:optab><mode>3):
            Add combine splitters for *<LOGICAL:optab>_ashl<mode>3 with
            ZERO_EXTEND, SIGN_EXTEND or AND.

            * gcc.target/aarch64/pr100056.c: New test.

[Bug target/100056] [9/10 Regression] orr + lsl vs. [us]bfiz

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[9/10/11 Regression]  orr + |[9/10 Regression]  orr +
                   |lsl vs. [us]bfiz            |lsl vs. [us]bfiz

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.  Backports unlikely.

[Bug target/100056] [9/10 Regression] orr + lsl vs. [us]bfiz

[luc.vanoostenryck at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #11 from Luc Van Oostenryck <luc.vanoostenryck at gmail dot com> ---
Works nicely now.
Thank you.

[Bug target/100056] [9/10 Regression] orr + lsl vs. [us]bfiz

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.4                         |9.5

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9.4 is being released, retargeting bugs to GCC 9.5.

[Bug target/100056] [9/10 Regression] orr + lsl vs. [us]bfiz

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rsandifo at gcc dot gnu.org

--- Comment #13 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
The patch is no longer effective after
g:96146e61cd7aee62c21c2845916ec42152918ab7, since e.g.:

int
or_shift_u3a (unsigned i)
{
  i &= 7;
  return i | (i << 11);
}

now gets folded to:

  i_2 = i_1(D) & 7;
  _5 = i_2 * 2049;

We then expand the multiplication as plus+shift rather
than ior+shift.

[Bug target/100056] [9/10 Regression] orr + lsl vs. [us]bfiz

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:25332d2325c720f584444c3858efdb85b8a3c06a

commit r12-7259-g25332d2325c720f584444c3858efdb85b8a3c06a
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Wed Feb 16 10:21:13 2022 +0000

    aarch64: Extend PR100056 patterns to +

    pr100056.c contains things like:

        int
        or_shift_u3a (unsigned i)
        {
          i &= 7;
          return i | (i << 11);
        }

    After g:96146e61cd7aee62c21c2845916ec42152918ab7, the preferred
    gimple representation of this is a multiplication:

      i_2 = i_1(D) & 7;
      _5 = i_2 * 2049;

    Expand then open-codes the multiplication back to individual shifts,
    but (of course) it uses + rather than | to combine the shifts.
    This means that we end up with the RTL equivalent of:

      i + (i << 11)

    I wondered about canonicalising the + to | (*back* to | in this case)
    when the operands have no set bits in common and when one of the
    operands is &, | or ^, but that didn't seem to be a popular idea when
    I asked on IRC.  The feeling seemed to be that + is inherently simpler
    than |, so we shouldn't be âsimplifyingâ the other way.

    This patch therefore adjusts the PR100056 patterns to handle +
    as well as |, in cases where the operands are provably disjoint.

    For:

        int
        or_shift_u8 (unsigned char i)
        {
          return i | (i << 11);
        }

    the instructions:

        2: r95:SI=zero_extend(x0:QI)
          REG_DEAD x0:QI
        7: r98:SI=r95:SI<<0xb

    are combined into:

        (parallel [
            (set (reg:SI 98)
                 (and:SI (ashift:SI (reg:SI 0 x0 [ i ])
                                    (const_int 11 [0xb]))
                         (const_int 522240 [0x7f800])))
            (set (reg/v:SI 95 [ i ])
                 (zero_extend:SI (reg:QI 0 x0 [ i ])))
        ])

    which fails to match, but which is then split into its individual
    (independent) sets.  Later the zero_extend is combined with the add
    to get an ADD UXTB:

        (set (reg:SI 99)
             (plus:SI (zero_extend:SI (reg:QI 0 x0 [ i ]))
                      (reg:SI 98)))

    This means that there is never a 3-insn combo to match the split
    against.  The end result is therefore:

            ubfiz   w1, w0, 11, 8
            add     w0, w1, w0, uxtb

    This is a bit redundant, since it's doing the zero_extend twice.
    It is at least 2 instructions though, rather than the 3 that we
    had before the original patch for PR100056.  or_shift_u8_asm is
    affected similarly.

    The net effect is that we do still have 2 UBFIZs, but we're at
    least back down to 2 instructions per function, as for GCC 11.
    I think that's good enough for now.

    There are probably other instructions that should be extended
    to support + as well as | (e.g. the EXTR ones), but those aren't
    regressions and so are GCC 13 material.

    gcc/
            PR target/100056
            * config/aarch64/iterators.md (LOGICAL_OR_PLUS): New iterator.
            * config/aarch64/aarch64.md: Extend the PR100056 patterns
            to handle plus in the same way as ior, if the operands have
            no set bits in common.

    gcc/testsuite/
            PR target/100056
            * gcc.target/aarch64/pr100056.c: XFAIL the original UBFIZ test
            and instead expect two UBFIZs + two ADD UXTBs.

[Bug target/100056] [10 Regression] orr + lsl vs. [us]bfiz

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug target/100056] [10 Regression] orr + lsl vs. [us]bfiz

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug target/100056] [10 Regression] orr + lsl vs. [us]bfiz

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100056

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
   Target Milestone|10.5                        |11.0
         Resolution|---                         |FIXED
      Known to fail|                            |10.5.0

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed in GCC 11+