[Bug target/62262] New: aarch64 gcc generates invalid assembler

[Bug target/62262] New: aarch64 gcc generates invalid assembler

[carrot at google dot com]

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

            Bug ID: 62262
           Summary: aarch64 gcc generates invalid assembler
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: carrot at google dot com
            Target: aarch64

Compile following source code with options -fprofile-use -O2


static inline int CLZ(int mask) {
   return mask ? __builtin_clz(mask) : 32;
}

int foo(int value)
{
    if (value == 0)
        return 0;

    int bias = CLZ(value);
    value >>= bias;
    int zeros = CLZ(value << 1);
    value <<= zeros;

    int packed = (unsigned)(value << 9) >> 9;
    return packed;
}

Without any actual profiling data, trunk gcc generates

/tmp/cctLq1F0.s: Assembler messages:
/tmp/cctLq1F0.s:20: Error: immediate value out of range 0 to 31 at operand 3 --
`ubfiz w0,w2,32,23'

The generated assembler code is:

foo:
    cbz    w0, .L4
    clz    w1, w0
    asr    w2, w0, w1
    adds    w0, w2, w2
    beq    .L3
    clz    w3, w0
    lsl    w4, w2, w3
    and    w0, w4, 8388607
    ret
.L3:
    ubfiz    w0, w2, 32, 23       // Wrong code
    ret
.L4:
    mov    w0, 0
    ret

4.9 gcc generates the same error.

[Bug target/62262] aarch64 gcc generates invalid assembler

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-08-26
     Ever confirmed|0                           |1

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(insn 27 26 40 5 (set (reg:SI 73 [ D.2590 ])
        (and:SI (ashift:SI (reg/v:SI 74 [ value ])
                (const_int 32 [0x20]))
            (const_int 8388607 [0x7fffff]))) t7.c:13 611 {*andim_ashiftsi_bfiz}
     (expr_list:REG_DEAD (reg/v:SI 74 [ value ])
        (nil)))

Confirmed.

  "exact_log2 ((INTVAL (operands[3]) >> INTVAL (operands[2])) + 1) >= 0
   && (INTVAL (operands[3]) & ((1 << INTVAL (operands[2])) - 1)) == 0"


In fact we invoke undefined behavior inside the compiler too due to the shift
there.

[Bug target/62262] aarch64 gcc generates invalid assembler

[amker at gcc dot gnu.org]

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

amker at gcc dot gnu.org changed:

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

--- Comment #2 from amker at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #1)
> (insn 27 26 40 5 (set (reg:SI 73 [ D.2590 ])
>         (and:SI (ashift:SI (reg/v:SI 74 [ value ])
>                 (const_int 32 [0x20]))
>             (const_int 8388607 [0x7fffff]))) t7.c:13 611
> {*andim_ashiftsi_bfiz}
>      (expr_list:REG_DEAD (reg/v:SI 74 [ value ])
>         (nil)))
> 
> Confirmed.
> 
>   "exact_log2 ((INTVAL (operands[3]) >> INTVAL (operands[2])) + 1) >= 0
>    && (INTVAL (operands[3]) & ((1 << INTVAL (operands[2])) - 1)) == 0"
> 
> 
> In fact we invoke undefined behavior inside the compiler too due to the
> shift there.

Since it's undefined code, how should we handle it in GCC?  Should we give
warning messages as accurate as possible?  But that sounds impractical either,
since "value << 1" and "value <<= zeros" could be undefined too.

[Bug target/62262] aarch64 gcc generates invalid assembler

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to amker from comment #2)
> (In reply to Andrew Pinski from comment #1)
> > (insn 27 26 40 5 (set (reg:SI 73 [ D.2590 ])
> >         (and:SI (ashift:SI (reg/v:SI 74 [ value ])
> >                 (const_int 32 [0x20]))
> >             (const_int 8388607 [0x7fffff]))) t7.c:13 611
> > {*andim_ashiftsi_bfiz}
> >      (expr_list:REG_DEAD (reg/v:SI 74 [ value ])
> >         (nil)))
> > 
> > Confirmed.
> > 
> >   "exact_log2 ((INTVAL (operands[3]) >> INTVAL (operands[2])) + 1) >= 0
> >    && (INTVAL (operands[3]) & ((1 << INTVAL (operands[2])) - 1)) == 0"
> > 
> > 
> > In fact we invoke undefined behavior inside the compiler too due to the
> > shift there.
> 
> Since it's undefined code, how should we handle it in GCC?  Should we give
> warning messages as accurate as possible?  But that sounds impractical
> either, since "value << 1" and "value <<= zeros" could be undefined too.

Look at how other targets handle cases like this they reject patterns like
this. I can fix this but not until next week.

[Bug target/62262] aarch64 gcc generates invalid assembler

[amker at gcc dot gnu.org]

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

--- Comment #4 from amker at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #3)
> (In reply to amker from comment #2)
> > (In reply to Andrew Pinski from comment #1)
> > > (insn 27 26 40 5 (set (reg:SI 73 [ D.2590 ])
> > >         (and:SI (ashift:SI (reg/v:SI 74 [ value ])
> > >                 (const_int 32 [0x20]))
> > >             (const_int 8388607 [0x7fffff]))) t7.c:13 611
> > > {*andim_ashiftsi_bfiz}
> > >      (expr_list:REG_DEAD (reg/v:SI 74 [ value ])
> > >         (nil)))
> > > 
> > > Confirmed.
> > > 
> > >   "exact_log2 ((INTVAL (operands[3]) >> INTVAL (operands[2])) + 1) >= 0
> > >    && (INTVAL (operands[3]) & ((1 << INTVAL (operands[2])) - 1)) == 0"
> > > 
> > > 
> > > In fact we invoke undefined behavior inside the compiler too due to the
> > > shift there.
> > 
> > Since it's undefined code, how should we handle it in GCC?  Should we give
> > warning messages as accurate as possible?  But that sounds impractical
> > either, since "value << 1" and "value <<= zeros" could be undefined too.
> 
> Look at how other targets handle cases like this they reject patterns like
> this. I can fix this but not until next week.

Thanks very much for explaining.

[Bug target/62262] aarch64 gcc generates invalid assembler

[pinskia at gcc dot gnu.org]

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Also the reason why it shows up in the first place is because the gimple level
tracer produces:
  <bb 7>:
  # iftmp.0_13 = PHI <32(3)>
  value_5 = value_3 << iftmp.0_13;
  packed_6 = value_5 & 8388607;

And never props the constant from the PHI node until RTL combine.

[Bug target/62262] aarch64 gcc generates invalid assembler

[amker at gcc dot gnu.org]

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

--- Comment #8 from amker at gcc dot gnu.org ---
(In reply to Carrot from comment #5)
> (In reply to amker from comment #2)
> > (In reply to Andrew Pinski from comment #1)
> > > (insn 27 26 40 5 (set (reg:SI 73 [ D.2590 ])
> > >         (and:SI (ashift:SI (reg/v:SI 74 [ value ])
> > >                 (const_int 32 [0x20]))
> > >             (const_int 8388607 [0x7fffff]))) t7.c:13 611
> > > {*andim_ashiftsi_bfiz}
> > >      (expr_list:REG_DEAD (reg/v:SI 74 [ value ])
> > >         (nil)))
> > > 
> > > Confirmed.
> > > 
> > >   "exact_log2 ((INTVAL (operands[3]) >> INTVAL (operands[2])) + 1) >= 0
> > >    && (INTVAL (operands[3]) & ((1 << INTVAL (operands[2])) - 1)) == 0"
> > > 
> > > 
> > > In fact we invoke undefined behavior inside the compiler too due to the
> > > shift there.
> > 
> > Since it's undefined code, how should we handle it in GCC?  Should we give
> > warning messages as accurate as possible?  But that sounds impractical
> > either, since "value << 1" and "value <<= zeros" could be undefined too.
> 
> Actually the original source code is guarded by assert, and the parameter
> passed to CLZ can be guaranteed not 0, so "value <<= zeros" is well defined
> in our original source code.

What I mean is by standard, E1<<E2 is undefined if E1 is of signed type and is
negative.  But that's another scenario, I suppose it won't happen in your case.
 Am I misreading?

[Bug target/62262] aarch64 gcc generates invalid assembler

[carrot at gcc dot gnu.org]

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

--- Comment #9 from amker at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #6)
> (In reply to Carrot from comment #5)
> > Actually the original source code is guarded by assert, and the parameter
> > passed to CLZ can be guaranteed not 0, so "value <<= zeros" is well defined
> > in our original source code.
> 
> The issue is the second CLZ (not the first one).  Though if I read the code
> correctly value<<1 can never be zero but GCC does not optimize away that
> check.

How is that?  Both "value >> bias" and "value << 1" are zero if value == 1 at
the entry, am I misunderstanding?

--- Comment #10 from carrot at gcc dot gnu.org ---
Author: carrot
Date: Wed Aug 27 16:48:09 2014
New Revision: 214578

URL: https://gcc.gnu.org/viewcvs?rev=214578&root=gcc&view=rev
Log:
    PR target/62262
    * config/aarch64/aarch64.md (*andim_ashift<mode>_bfiz): Check the shift
    amount before using it.

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


Added:
    trunk/gcc/testsuite/gcc.target/aarch64/pr62262.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64.md
    trunk/gcc/testsuite/ChangeLog

[Bug target/62262] aarch64 gcc generates invalid assembler

[carrot at gcc dot gnu.org]

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

--- Comment #11 from carrot at gcc dot gnu.org ---
Author: carrot
Date: Wed Aug 27 23:00:54 2014
New Revision: 214610

URL: https://gcc.gnu.org/viewcvs?rev=214610&root=gcc&view=rev
Log:
    PR target/62262
    * config/aarch64/aarch64.md (*andim_ashift<mode>_bfiz): Check the shift
    amount before using it.

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


Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/aarch64/pr62262.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/aarch64/aarch64.md
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog

[Bug target/62262] aarch64 gcc generates invalid assembler

[mshawcroft at gcc dot gnu.org]

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

mshawcroft at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |mshawcroft at gcc dot gnu.org
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.9.2

--- Comment #12 from mshawcroft at gcc dot gnu.org ---
This was resolved in trunk and 4.9 by Carrot.