[Bug target/46915] New: Wrong code is generated for conditional branch followed by zero length asm

[Bug target/46915] New: Wrong code is generated for conditional branch followed by zero length asm

[danglin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

           Summary: Wrong code is generated for conditional branch
                    followed by zero length asm
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: danglin@gcc.gnu.org
              Host: hppa*-*-*
            Target: hppa*-*-*
             Build: hppa*-*-*


Created attachment 22728
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22728
Preprocessed source

With gcc 4.4 and current head, we generate incorrect code for the function
add_to_swap at -O2:

int add_to_swap(struct page *page)
{
 swp_entry_t entry;
 int err;

 do { (void)(!PageLocked(page)); } while (0);
 do { (void)(!PageUptodate(page)); } while (0);

 entry = get_swap_page();
 ...

PageUptodate is:

static inline __attribute__((always_inline)) int PageUptodate(struct page
*page)
{
 int ret = test_bit(PG_uptodate, &(page)->flags);
# 297 "include/linux/page-flags.h"
 if (ret)
  __asm__ __volatile__("":::"memory");

 return ret;
}

The generated RTL is:

(jump_insn 13 10 14 (set (pc)
        (if_then_else (eq (zero_extract:SI (reg:SI 28 %r28 [orig:100 D.22506 ]
[
100])
                    (const_int 1 [0x1])
                    (const_int 28 [0x1c]))
                (const_int 0 [0]))
            (label_ref 16)
            (pc))) include/linux/page-flags.h:297 29 {*pa.md:1610}
     (expr_list:REG_BR_PRED (const_int 5 [0x5])
        (expr_list:REG_DEAD (reg:SI 28 %r28 [orig:100 D.22506 ] [100])
            (expr_list:REG_BR_PROB (const_int 5000 [0x1388])
                (nil))))
 -> 16)

(note 14 13 15 [bb 3] NOTE_INSN_BASIC_BLOCK)

(insn:TI 15 14 16 (parallel [
            (asm_operands/v ("") ("") 0 []
                 []
                 [] mm/swap_state.c:185)
            (clobber (mem:BLK (scratch) [0 A8]))
        ]) include/linux/page-flags.h:298 -1
    (nil))

(code_label 16 15 17 15 "" [1 uses])

(note 17 16 19 [bb 4] NOTE_INSN_BASIC_BLOCK)

(note 19 17 107 NOTE_INSN_DELETED)

(insn 107 19 122 (sequence [
            (call_insn:TI 18 19 4 (parallel [
                        (set (reg:SI 28 %r28)
                            (call (mem:SI (symbol_ref/v:SI ("@get_swap_page")
[f
lags 0x41]  <function_decl 0x40ae6280 get_swap_page>) [0 S4 A32])
...

This results in the following assembler:

globl add_to_swap
        .type   add_to_swap, @function
add_to_swap:
        .PROC
        .CALLINFO FRAME=64,CALLS,SAVE_RP,ENTRY_GR=5
        .ENTRY
        stw %r2,-20(%r30)
        stwm %r5,64(%r30)
        stw %r3,-56(%r30)
        copy %r26,%r3
        stw %r4,-60(%r30)
        ldw 0(%r26),%r28
        ldw 0(%r26),%r28
        bb,>=,n %r28,28,.L15
.L15:
        bl get_swap_page,%r2
        ldi 0,%r4

As can be seen, the bb instruction branches to its delay slot.  This
is not handled by the hardware.

The asm confuses the branch code generation.

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #1 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 02:45:37 UTC ---
Reduced testcase attached.

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #2 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 02:45:38 UTC ---
Created attachment 22731
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22731
xxx.c

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #3 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 02:49:57 UTC ---
Reduced testcase attached.

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #5 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 03:59:08 UTC ---
Testing attached change.  branch_to_delay_slot_p didn't correctly
handle asms, etc.  When it is fixed, branch_needs_nop_p isn't needed.
It also didn't handle all cases.

Dave

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #6 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 03:59:09 UTC ---
Created attachment 22734
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22734
pa.c.d

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[amylaar at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

Jorn Wolfgang Rennecke <amylaar at gcc dot gnu.org> changed:

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

--- Comment #7 from Jorn Wolfgang Rennecke <amylaar at gcc dot gnu.org> 2010-12-13 09:34:49 UTC ---
(In reply to comment #6)
> Created attachment 22734 [details]
> pa.c.d

This will break if you have an asm that gcc thinks is non-zero in length,
but which is, in fact, zero length.  E.g you might have a comment explaining
why no code is needed, and gcc sees a line, which it equates to an insn.

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-12-13 10:59:28 UTC ---
Yeah, the insn count for asm is just a guess.  You should never put inline asm
into a delay slot, you really don't know how big it is or if it is suitable for
the delay slot.
I guess the bug is in checking just for ASM_INPUT, that should be also testing
extract_asm_operands () != NULL_RTX.

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #9 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 12:44:21 UTC ---
> Yeah, the insn count for asm is just a guess.  You should never put inline asm
> into a delay slot, you really don't know how big it is or if it is suitable for
> the delay slot.
> I guess the bug is in checking just for ASM_INPUT, that should be also testing
> extract_asm_operands () != NULL_RTX.

Thanks, I'll look at that.  However, I think branch_to_delay_slot_p can
be improved.  It probably should also check for asms and return FALSE
if it finds one.

Dave

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #10 from dave at hiauly1 dot hia.nrc.ca 2010-12-13 12:51:51 UTC ---
> Thanks, I'll look at that.  However, I think branch_to_delay_slot_p can
> be improved.  It probably should also check for asms and return FALSE
> if it finds one.

Another related issue is the "useskip" check.  It also needs to check
for asms.

Dave

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[danglin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #11 from John David Anglin <danglin at gcc dot gnu.org> 2010-12-18 17:10:37 UTC ---
Author: danglin
Date: Sat Dec 18 17:10:34 2010
New Revision: 168026

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168026
Log:
    PR target/46915
    * config/pa/pa.c (branch_to_delay_slot_p): Use next_active_insn instead
    of next_real_insn.  Search forward checking for both ASM_INPUT and
    ASM_OPERANDS asms until exit condition is found.
    (branch_needs_nop_p): Likewise.
    (use_skip_p): New function.
    (output_cbranch): Use use_skip_p.
    (output_bb, output_bvb): Likewise.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/pa/pa.c

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[eike@sf-mail.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #12 from Rolf Eike Beer <eike@sf-mail.de> 2010-12-18 22:27:40 UTC ---
Any chance this gets backported?

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[dave at hiauly1 dot hia.nrc.ca]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #13 from dave at hiauly1 dot hia.nrc.ca 2010-12-18 22:42:13 UTC ---
> Any chance this gets backported?

It's not a regression, but I think it should be backported since it breaks
Linux in a somewhat random manner.  I'm currently testing the change on the
4.5 branch.

Dave

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[danglin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #14 from John David Anglin <danglin at gcc dot gnu.org> 2010-12-19 19:50:20 UTC ---
Author: danglin
Date: Sun Dec 19 19:50:17 2010
New Revision: 168072

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168072
Log:
    Backport from mainline:
    2010-12-18  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

    PR target/46915
    * config/pa/pa.c (branch_to_delay_slot_p): Use next_active_insn instead
    of next_real_insn.  Search forward checking for both ASM_INPUT and
    ASM_OPERANDS asms until exit condition is found.
    (branch_needs_nop_p): Likewise.
    (use_skip_p): New function.
    (output_cbranch): Use use_skip_p.
    (output_bb, output_bvb): Likewise.


Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/config/pa/pa.c

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[danglin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #15 from John David Anglin <danglin at gcc dot gnu.org> 2010-12-22 18:47:04 UTC ---
Author: danglin
Date: Wed Dec 22 18:46:57 2010
New Revision: 168177

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168177
Log:
    Backport from mainline:
    2010-12-18  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

    PR target/46915
    * config/pa/pa.c (branch_to_delay_slot_p): Use next_active_insn instead
    of next_real_insn.  Search forward checking for both ASM_INPUT and
    ASM_OPERANDS asms until exit condition is found.
    (branch_needs_nop_p): Likewise.
    (use_skip_p): New function.
    (output_cbranch): Use use_skip_p.
    (output_bb, output_bvb): Likewise.

    2009-06-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

    PR target/40468
    * pa.c (branch_to_delay_slot_p, branch_needs_nop_p): New functions.
    (output_cbranch): Use new functions.
    (output_bb, output_bvb, output_dbra, output_movb): Likewise.


Modified:
    branches/gcc-4_4-branch/gcc/ChangeLog
    branches/gcc-4_4-branch/gcc/config/pa/pa.c

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[danglin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

--- Comment #16 from John David Anglin <danglin at gcc dot gnu.org> 2010-12-22 18:49:31 UTC ---
Author: danglin
Date: Wed Dec 22 18:49:24 2010
New Revision: 168178

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168178
Log:
    Backport from mainline:
    2010-12-18  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

    PR target/46915
    * config/pa/pa.c (branch_to_delay_slot_p): Use next_active_insn instead
    of next_real_insn.  Search forward checking for both ASM_INPUT and
    ASM_OPERANDS asms until exit condition is found.
    (branch_needs_nop_p): Likewise.
    (use_skip_p): New function.
    (output_cbranch): Use use_skip_p.
    (output_bb, output_bvb): Likewise.

    2009-06-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

    PR target/40468
    * pa.c (branch_to_delay_slot_p, branch_needs_nop_p): New functions.
    (output_cbranch): Use new functions.
    (output_bb, output_bvb, output_dbra, output_movb): Likewise.


Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/config/pa/pa.c

[Bug target/46915] Wrong code is generated for conditional branch followed by zero length asm

[danglin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46915

John David Anglin <danglin at gcc dot gnu.org> changed:

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

--- Comment #17 from John David Anglin <danglin at gcc dot gnu.org> 2010-12-22 18:58:20 UTC ---
Fixed.