[Bug target/98681] New: aarch64: Invalid ubfiz instruction rejected by assembler

[Bug target/98681] New: aarch64: Invalid ubfiz instruction rejected by assembler

[acoplan at gcc dot gnu.org]

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

            Bug ID: 98681
           Summary: aarch64: Invalid ubfiz instruction rejected by
                    assembler
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acoplan at gcc dot gnu.org
  Target Milestone: ---

AArch64 GCC generates an invalid ubfiz instruction for the following testcase:

extern unsigned char a[];
int b;
void c() {
  for (int d; d < 120; d += 6)
    a[d] = b << -64;
}

$ aarch64-elf-gcc -c test.c -O3
test.c: In function 'c':
test.c:5:14: warning: left shift count is negative [-Wshift-count-negative]
    5 |     a[d] = b << -64;
      |              ^~
/tmp/ccelsQle.s: Assembler messages:
/tmp/ccelsQle.s:14: Error: immediate value out of range 0 to 31 at operand 3 --
`ubfiz w1,w1,-64,8'

Although this testcase invokes UB, it was reduced from a testcase which
ostensibly does not. In any case, we should not generate invalid assembly here.

[Bug target/98681] aarch64: Invalid ubfiz instruction rejected by assembler

[acoplan at gcc dot gnu.org]

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

Alex Coplan <acoplan at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
                 CC|                            |ktkachov at gcc dot gnu.org
            Summary|aarch64: Invalid ubfiz      |[8/9/10/11 Regression]
                   |instruction rejected by     |aarch64: Invalid ubfiz
                   |assembler                   |instruction rejected by
                   |                            |assembler
           Priority|P3                          |P2
   Last reconfirmed|                            |2021-01-14

--- Comment #1 from ktkachov at gcc dot gnu.org ---
Confirmed. GCC 5 didn't generate the bogus assembly.
I suppose the predicate and/or constraint of *andim_ashift<mode>_bfiz needs
tightening

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.0                        |8.5

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=87511

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87511 .

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Adjusted testcase (-O2):

__attribute__((noipa)) int
foo (int x)
{
  if (x > 32)
    return (x << -64) & 255;
  else
    return x;
}

int
main ()
{
  if (foo (32) != 32 || foo (-150) != -150)
    __builtin_abort ();
  return 0;
}

This doesn't invoke UB.

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

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

Untested fix.

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[rearnsha at gcc dot gnu.org]

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

--- Comment #5 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
Patch looks generally sensible, but I think all the INTVALs in that expression
should be converted to UINTVAL.  The mask, in particular, is unsigned and it is
weird that one moment we're using a unsigned value and the next we're using a
signed value for shift_amt.

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For shft_amnt that single UINTVAL check will make sure it is not negative, so
whether the rest uses INTVAL or UINTVAL is then irrelevant.
Up to you as maintainer what you want there.
As for mask, let me check the exact insn behavior.

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, for the mask, the question is if we should or shouldn't handle e.g.
unsigned
foo (unsigned int x)
{
  return (x << 13) & (-1U << 13);
}
or
unsigned
bar (unsigned int x)
{
  return (x << 0) & -1U;
}
I think the instruction in theory can handle those corner-cases, 
as lsb is [0, 31] and width [1, 32-lsb], but the optimizers (both GIMPLE and
simplify-rtx during combine) will optimize that away (the useless masking in
both cases and useless shift in the second case).
So the case where mask is negative is only theoretical whether it should or
shouldn't match, that
exact_log2 ((INTVAL (mask) >> INTVAL (shft_amnt)) + 1)
for negative INTVAL (mask) will result in -1 + 1 for the masks with bits
starting with shft_amnt set and smaller clear and exact_log2 (0) is -1, thus it
fail, while if UINTVAL it could succeed.  E.g. that hypothetical -1U << 13 mask
is in CONST_INT_P -1 << 13 so in UINTVAL (0xffffffffffffe000 >> 13) + 1 is a
power of two.  But e.g. for shift count 0, -1 mask would be (0xffffffffffffffff
>> 0) + 1,
so 0 and fail anyway.

On the other side, I'm not sure if the define_insn will work correctly when
mask is negative - consider again the (x << 13) & (-1U << 13).
The instruction uses %P3 which uses
    case 'P':
      if (!CONST_INT_P (x))
        {
          output_operand_lossage ("invalid operand for '%%%c'", code);
          return;
        }

      asm_fprintf (f, "%u", popcount_hwi (INTVAL (x)));
but the constant is not -1U << 13, but -1 << 13 and so %P3 will not print
ubfiz   w1, w0, 13, 19
that should be printed, but
ubfiz   w1, w0, 13, 51
on which gas will fail again:
pr98681.s: Assembler messages:
pr98681.s:11: Error: immediate value out of range 1 to 32 at operand 4 --
`ubfiz w1,w0,13,51'
So, I think it is better to keep INTVAL (mask) as is.

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

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

Therefore, if you want to use UINTVAL in that function wherever possible, we
can, but we need to add INTVAL (mask) > 0 check to catch the problems with
negative mask %P printing.  I'm already bootstrapping/regtesting the earlier
patch, but can throw this one in too.

[Bug target/98681] [8/9/10/11 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[cvs-commit at gcc dot gnu.org]

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

--- 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:17ad8cdebe65b47d257d85849747b806af0a85fd

commit r11-6914-g17ad8cdebe65b47d257d85849747b806af0a85fd
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 26 14:48:26 2021 +0100

    aarch64: Tighten up checks for ubfix [PR98681]

    The testcase in the patch doesn't assemble, because the instruction
requires
    that the penultimate operand (lsb) range is [0, 32] (or [0, 64]) and the
last
    operand's range is [1, 32 - lsb] (or [1, 64 - lsb]).
    The INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) will accept the lsb
operand
    to be in range [MIN, 32] (or [MIN, 64]) and then we invoke UB in the
    compiler and sometimes it will make it through.
    The patch changes all the INTVAL uses in that function to UINTVAL,
    which isn't strictly necessary, but can be done (e.g. after the
    UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) check we know it is not
    negative and thus INTVAL (shft_amnt) and UINTVAL (shft_amnt) then behave
the
    same.  But, I had to add INTVAL (mask) > 0 check in that case, otherwise we
    risk (hypothetically) emitting instruction that doesn't assemble.
    The problem is with masks that have the MSB bit set, while the instruction
    can handle those, e.g.
    ubfiz w1, w0, 13, 19
    will do
    (w0 << 13) & 0xffffe000
    in RTL we represent SImode constants with MSB set as negative
HOST_WIDE_INT,
    so it will actually be HOST_WIDE_INT_C (0xffffffffffffe000), and
    the instruction uses %P3 to print the last operand, which calls
    asm_fprintf (f, "%u", popcount_hwi (INTVAL (x)))
    to print that.  But that will not print 19, but 51 instead, will include
    there also all the copies of the sign bit.
    Not supporting those masks with MSB set isn't a big loss though, they
really
    shouldn't appear normally, as both GIMPLE and RTL optimizations should
    optimize those away (one isn't masking any bits off with such masks, so
    just w0 << 13 will do too).

    2021-01-26  Jakub Jelinek  <jakub@redhat.com>

            PR target/98681
            * config/aarch64/aarch64.c (aarch64_mask_and_shift_for_ubfiz_p):
            Use UINTVAL (shft_amnt) and UINTVAL (mask) instead of INTVAL
(shft_amnt)
            and INTVAL (mask).  Add && INTVAL (mask) > 0 condition.

            * gcc.c-torture/execute/pr98681.c: New test.

[Bug target/98681] [8/9/10 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[8/9/10/11 Regression]      |[8/9/10 Regression]
                   |aarch64: Invalid ubfiz      |aarch64: Invalid ubfiz
                   |instruction rejected by     |instruction rejected by
                   |assembler                   |assembler

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

[Bug target/98681] [8/9/10 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:fb09d7242a25971b275292332337a56b86637f2c

commit r10-9321-gfb09d7242a25971b275292332337a56b86637f2c
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 26 14:48:26 2021 +0100

    aarch64: Tighten up checks for ubfix [PR98681]

    The testcase in the patch doesn't assemble, because the instruction
requires
    that the penultimate operand (lsb) range is [0, 32] (or [0, 64]) and the
last
    operand's range is [1, 32 - lsb] (or [1, 64 - lsb]).
    The INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) will accept the lsb
operand
    to be in range [MIN, 32] (or [MIN, 64]) and then we invoke UB in the
    compiler and sometimes it will make it through.
    The patch changes all the INTVAL uses in that function to UINTVAL,
    which isn't strictly necessary, but can be done (e.g. after the
    UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) check we know it is not
    negative and thus INTVAL (shft_amnt) and UINTVAL (shft_amnt) then behave
the
    same.  But, I had to add INTVAL (mask) > 0 check in that case, otherwise we
    risk (hypothetically) emitting instruction that doesn't assemble.
    The problem is with masks that have the MSB bit set, while the instruction
    can handle those, e.g.
    ubfiz w1, w0, 13, 19
    will do
    (w0 << 13) & 0xffffe000
    in RTL we represent SImode constants with MSB set as negative
HOST_WIDE_INT,
    so it will actually be HOST_WIDE_INT_C (0xffffffffffffe000), and
    the instruction uses %P3 to print the last operand, which calls
    asm_fprintf (f, "%u", popcount_hwi (INTVAL (x)))
    to print that.  But that will not print 19, but 51 instead, will include
    there also all the copies of the sign bit.
    Not supporting those masks with MSB set isn't a big loss though, they
really
    shouldn't appear normally, as both GIMPLE and RTL optimizations should
    optimize those away (one isn't masking any bits off with such masks, so
    just w0 << 13 will do too).

    2021-01-26  Jakub Jelinek  <jakub@redhat.com>

            PR target/98681
            * config/aarch64/aarch64.c (aarch64_mask_and_shift_for_ubfiz_p):
            Use UINTVAL (shft_amnt) and UINTVAL (mask) instead of INTVAL
(shft_amnt)
            and INTVAL (mask).  Add && INTVAL (mask) > 0 condition.

            * gcc.c-torture/execute/pr98681.c: New test.

[Bug target/98681] [8/9 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[8/9/10 Regression]         |[8/9 Regression] aarch64:
                   |aarch64: Invalid ubfiz      |Invalid ubfiz instruction
                   |instruction rejected by     |rejected by assembler
                   |assembler                   |

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 10.3+ too.

[Bug target/98681] [8/9 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:e3dc765eb4556b78fe52d32be9858f2805b4488d

commit r9-9410-ge3dc765eb4556b78fe52d32be9858f2805b4488d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 26 14:48:26 2021 +0100

    aarch64: Tighten up checks for ubfix [PR98681]

    The testcase in the patch doesn't assemble, because the instruction
requires
    that the penultimate operand (lsb) range is [0, 32] (or [0, 64]) and the
last
    operand's range is [1, 32 - lsb] (or [1, 64 - lsb]).
    The INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) will accept the lsb
operand
    to be in range [MIN, 32] (or [MIN, 64]) and then we invoke UB in the
    compiler and sometimes it will make it through.
    The patch changes all the INTVAL uses in that function to UINTVAL,
    which isn't strictly necessary, but can be done (e.g. after the
    UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) check we know it is not
    negative and thus INTVAL (shft_amnt) and UINTVAL (shft_amnt) then behave
the
    same.  But, I had to add INTVAL (mask) > 0 check in that case, otherwise we
    risk (hypothetically) emitting instruction that doesn't assemble.
    The problem is with masks that have the MSB bit set, while the instruction
    can handle those, e.g.
    ubfiz w1, w0, 13, 19
    will do
    (w0 << 13) & 0xffffe000
    in RTL we represent SImode constants with MSB set as negative
HOST_WIDE_INT,
    so it will actually be HOST_WIDE_INT_C (0xffffffffffffe000), and
    the instruction uses %P3 to print the last operand, which calls
    asm_fprintf (f, "%u", popcount_hwi (INTVAL (x)))
    to print that.  But that will not print 19, but 51 instead, will include
    there also all the copies of the sign bit.
    Not supporting those masks with MSB set isn't a big loss though, they
really
    shouldn't appear normally, as both GIMPLE and RTL optimizations should
    optimize those away (one isn't masking any bits off with such masks, so
    just w0 << 13 will do too).

    2021-01-26  Jakub Jelinek  <jakub@redhat.com>

            PR target/98681
            * config/aarch64/aarch64.c (aarch64_mask_and_shift_for_ubfiz_p):
            Use UINTVAL (shft_amnt) and UINTVAL (mask) instead of INTVAL
(shft_amnt)
            and INTVAL (mask).  Add && INTVAL (mask) > 0 condition.

            * gcc.c-torture/execute/pr98681.c: New test.

    (cherry picked from commit fb09d7242a25971b275292332337a56b86637f2c)

[Bug target/98681] [8/9 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:4e4a5809586a345e12aed7a65e89551d6bdfbda3

commit r8-10877-g4e4a5809586a345e12aed7a65e89551d6bdfbda3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Jan 26 14:48:26 2021 +0100

    aarch64: Tighten up checks for ubfix [PR98681]

    The testcase in the patch doesn't assemble, because the instruction
requires
    that the penultimate operand (lsb) range is [0, 32] (or [0, 64]) and the
last
    operand's range is [1, 32 - lsb] (or [1, 64 - lsb]).
    The INTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) will accept the lsb
operand
    to be in range [MIN, 32] (or [MIN, 64]) and then we invoke UB in the
    compiler and sometimes it will make it through.
    The patch changes all the INTVAL uses in that function to UINTVAL,
    which isn't strictly necessary, but can be done (e.g. after the
    UINTVAL (shft_amnt) < GET_MODE_BITSIZE (mode) check we know it is not
    negative and thus INTVAL (shft_amnt) and UINTVAL (shft_amnt) then behave
the
    same.  But, I had to add INTVAL (mask) > 0 check in that case, otherwise we
    risk (hypothetically) emitting instruction that doesn't assemble.
    The problem is with masks that have the MSB bit set, while the instruction
    can handle those, e.g.
    ubfiz w1, w0, 13, 19
    will do
    (w0 << 13) & 0xffffe000
    in RTL we represent SImode constants with MSB set as negative
HOST_WIDE_INT,
    so it will actually be HOST_WIDE_INT_C (0xffffffffffffe000), and
    the instruction uses %P3 to print the last operand, which calls
    asm_fprintf (f, "%u", popcount_hwi (INTVAL (x)))
    to print that.  But that will not print 19, but 51 instead, will include
    there also all the copies of the sign bit.
    Not supporting those masks with MSB set isn't a big loss though, they
really
    shouldn't appear normally, as both GIMPLE and RTL optimizations should
    optimize those away (one isn't masking any bits off with such masks, so
    just w0 << 13 will do too).

    2021-01-26  Jakub Jelinek  <jakub@redhat.com>

            PR target/98681
            * config/aarch64/aarch64.c (aarch64_mask_and_shift_for_ubfiz_p):
            Use UINTVAL (shft_amnt) and UINTVAL (mask) instead of INTVAL
(shft_amnt)
            and INTVAL (mask).  Add && INTVAL (mask) > 0 condition.

            * gcc.c-torture/execute/pr98681.c: New test.

    (cherry picked from commit fb09d7242a25971b275292332337a56b86637f2c)

[Bug target/98681] [8/9 Regression] aarch64: Invalid ubfiz instruction rejected by assembler

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.