[Bug target/53975] New: [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[Bug target/53975] New: [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[jakub at jermar dot eu]

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

             Bug #: 53975
           Summary: [ia64] Target register of a speculative load moved to
                    a branch register prior to the chk.s instruction
    Classification: Unclassified
           Product: gcc
           Version: 4.7.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jakub@jermar.eu


When building HelenOS/ia64 with gcc 4.7.1, we hit a problem in this code
sequence:

 4404570:       09 70 00 42 38 10       [MMI]       ld8.s r14=[r33]
 4404576:       a0 04 90 70 20 20                   ld8.s r74=[r36]
 440457c:       19 00 00 90                         mov r73=1;;
 4404580:       11 38 00 10 06 39       [MIB]       cmp.eq p7,p6=0,r8
 4404586:       60 70 04 80 83 03                   mov b6=r14
 440458c:       a0 fd ff 4b                   (p07) br.cond.dpnt.few 4404320
<printf_core+0x19e0>;;
 4404590:       c2 40 8a 19 00 21       [MII] (p06) adds r72=98,r12
 4404596:       00 00 00 02 00 40                   nop.i 0x0;;
 440459c:       a3 04 00 01                         chk.s.i r74,4404730
<printf_core+0x1df0>
 44045a0:       08 b8 38 00 40 04       [MMI]       chk.s.m r14,4404710
<printf_core+0x1dd0>
 44045a6:       00 00 00 02 00 00                   nop.m 0x0
 44045ac:       00 00 04 00                         nop.i 0x0
 44045b0:       11 00 00 00 01 00       [MIB]       nop.m 0x0
 44045b6:       00 00 00 02 00 00                   nop.i 0x0
 44045bc:       68 00 80 10                         br.call.sptk.many b0=b6;;

Note that r14 is loaded speculatively and thus can have its NaT bit set as a
result of a deferred exception. This is checked and, if necessary, recovered by
the chk.s instruction at 0x44045a0. The problem seems to be that r14 is moved
to b6 at 0x4404586 prior to the chk.s instruction. If r14 indeed has the NaT
bit set, the instruction will generate the NaT Consumption vector exception.
Seems to me that the mov b6=r14 instruction should have been scheduled only
after the chk.s instruction when we are sure r14's NaT is cleared.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2012-07-15
                 CC|                            |steven at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-15 20:56:51 UTC ---
Without a test case on a platform that is supported in GCC, there isn't much
anyone can do to help. Can you reproduce this on linux or hpux?

BTW, are there plans to contribute HelenOS support to FSF GCC?

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

--- Comment #2 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-15 21:08:30 UTC ---
FWIW, as far as I understand the ia64 data speculation semantics, it is OK to
use a speculated load, but the check must then be done on the use, i.e. in t
his case chk.s b6.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

--- Comment #3 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-15 21:17:34 UTC ---
What does the recovery code at 4404710 look like?

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

--- Comment #4 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-15 22:52:31 UTC ---
Ah, of course the "move branch register" instruction faults if the NaT bit of
the source register is set. So the recovery code is irrelevant, and this could
be a GCC bug. Need a test case to investigate, though...

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[jakub at jermar dot eu]

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

--- Comment #5 from Jakub Jermar <jakub at jermar dot eu> 2012-07-16 06:18:55 UTC ---
(In reply to comment #1)
> Without a test case on a platform that is supported in GCC, there isn't much
> anyone can do to help. Can you reproduce this on linux or hpux?
> 
> BTW, are there plans to contribute HelenOS support to FSF GCC?

Well, we are squatting on ia64-pc-linux-gnu-gcc to build HelenOS, so this is
reproducible on Linux (no HelenOS-specific support exists). The testcase is our
'image.boot' loader program, but it's kind of huge. Even the function which
contains this issue, i.e. printf_core(), is pretty huge and inlines lots of
other functions.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[jakub at jermar dot eu]

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

--- Comment #6 from Jakub Jermar <jakub at jermar dot eu> 2012-07-16 06:28:29 UTC ---
(In reply to comment #4)
> Ah, of course the "move branch register" instruction faults if the NaT bit of
> the source register is set. So the recovery code is irrelevant, and this could
> be a GCC bug. Need a test case to investigate, though...

Exactly. The problem is that the NaT bit cannot propagate any further when the
new destination is a branch register and so the exception can no longer remain
deferred.

As for the test case, once you have the toolchain in place, the easiest way to
reproduce this is simply to build HelenOS and disassemble the image.boot binary
around the addresses above. I'd be more than happy to provide assistance with
this. If tinkering with the entire HelenOS is not plausible, I can try to
separate at least the printf_core() into a separately buildable testcase.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[jakub at jermar dot eu]

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

--- Comment #7 from Jakub Jermar <jakub at jermar dot eu> 2012-07-16 21:25:47 UTC ---
Created attachment 27805
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27805
Reproducible testcase

Here is a trimmed down reproducible testcase which exhibits the problem. The
tarball contains a README file with instructions.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

--- Comment #8 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-18 23:13:32 UTC ---
Created attachment 27827
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27827
Somewhat reduced, preprocessed test case

Fails with a cross-compiler from x86_64 to ia64 with trunk r189633. Compile
with: "-O3 -ffreestanding -fno-builtin -std=gnu99 -fno-unwind-tables
-mfixed-range=f32-f127 -mno-pic -mno-sdata t.c -dAP -fdump-rtl-all".

Everything looks ok up to machine-reorg. The .218r.compgotos dump has:
(insn 2105 2104 2106 118 (set (reg:DI 122 r71)
        (mem/f:DI (reg/f:DI 34 r37 [1005]) [3 ps_90(D)->data+0 S8 A64])) t.c:95
6 {movdi_internal}
     (nil))

(insn 2106 2105 2107 118 (set (reg:DI 14 r14)
        (mem/f:DI (reg/v/f:DI 113 r33 [orig:610 ps ] [610]) [3
ps_90(D)->str_write+0 S8 A64])) t.c:95 6 {movdi_internal}
     (nil))

(insn 2107 2106 2108 118 (set (reg:DI 326 b6)
        (reg:DI 14 r14)) t.c:95 6 {movdi_internal}
     (expr_list:REG_DEAD (reg:DI 14 r14)
        (nil)))



But the .220r.mach dump has:
deleting insn with uid = 2106.
scanning new insn with uid = 3366.
...
(insn 3366 3398 3365 117 (set (reg:DI 14 r14)
        (unspec:DI [
                (mem/f:DI (reg/v/f:DI 113 r33 [orig:610 ps ] [610]) [3
ps_90(D)->str_write+0 S8 A64])
            ] UNSPEC_LDS)) 23 {movdi_speculative}
     (nil))
(insn 3365 3366 2104 117 (set (reg:DI 122 r71)
        (unspec:DI [
                (mem/f:DI (reg/f:DI 34 r37 [1005]) [3 ps_90(D)->data+0 S8 A64])
            ] UNSPEC_LDS)) 23 {movdi_speculative}
     (nil))
...
(insn 2107 892 893 117 (set (reg:DI 326 b6)
        (reg:DI 14 r14)) t.c:95 6 {movdi_internal}
     (expr_list:REG_DEAD (reg:DI 14 r14)
        (nil)))
...
(jump_insn:TI 3374 3410 3409 137 (set (pc)
        (if_then_else (ne (unspec [
                        (reg:DI 14 r14)
                    ] UNSPEC_CHKS)
                (const_int 0 [0]))
            (pc)
            (label_ref 3369))) t.c:95 101 {speculation_check_di}
     (nil)
 -> 3369)


This is done by the selective scheduler.

(Work-around: -fno-fselective-scheduling -fno-fselective-scheduling2)

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
                 CC|                            |amonakov at gcc dot gnu.org
      Known to fail|                            |4.7.0, 4.7.1, 4.8.0

--- Comment #9 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-18 23:18:02 UTC ---
Hello Alexander, this bug is a problem in the selective scheduler. A general
register holding an ld8.s load is assigned to a branch register before the
chk.s. But the branch registers have no NaT flag so the machine throws an
exception. Could you have a look at this please?

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

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

--- Comment #10 from Alexander Monakov <amonakov at gcc dot gnu.org> 2012-07-19 17:50:07 UTC ---
AFAICS speculating an assignment to a branch register should be rejected by
sched_insn_is_legitimate_for_speculation_p in sched-deps.c, but it does not
fall into any of the cases tested there.  Consequently, I suppose it's not
specific to sel-sched, just haifa schedules those regions differently and does
not produce those speculative loads.

I wonder how this bug managed to stay latent all these years :)

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[jakub at jermar dot eu]

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

--- Comment #11 from Jakub Jermar <jakub at jermar dot eu> 2012-07-19 18:06:16 UTC ---
For HelenOS, we started to hit this only after we switched from gcc 4.6.3 to
4.7.1.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[stevenb.gcc at gmail dot com]

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

--- Comment #12 from stevenb.gcc at gmail dot com <stevenb.gcc at gmail dot com> 2012-07-19 18:11:30 UTC ---
On Thu, Jul 19, 2012 at 7:50 PM, amonakov at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> I wonder how this bug managed to stay latent all these years :)

This is very simple: Nobody uses ia64 ;-)

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

Andrey Belevantsev <abel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot       |abel at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #13 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-07-20 09:13:28 UTC ---
The ia64 backend disallows any movements of instructions that depend on a
speculated insns with always returning false in insn_can_be_in_speculative_p,
and this later fails the ia64_speculate_insn hook.  Thus, sel-sched is likely
wrong to move this insn, I will take a look on Monday.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

--- Comment #14 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-07-24 09:22:14 UTC ---
The problem is that we don't handle this type of speculation well in sel-sched.
 While moving an insn through speculation check, it is hard to decide for us
whether it is safe, while it's simpler in haifa with explicit dependency lists.
 In this case, we have a true dependency to the load which in the old scheduler
would be moved to the check instead with the correct speculation bits, and thus
the backend will have a chance to allow or deny speculation (note that
currently it denis all of those "secondary" speculations anyways).  

In sel-sched we have only a check which does not longer write an address
register but just reads it, thus we no longer have a true dependency. 
Previosly, we just banned any insns that read registers to move through a
check, but later we (incorrectly) "fixed" it by only considering registers that
are written by the check.  So either we revert the patch at
http://gcc.gnu.org/viewcvs?view=revision&revision=177658 or we add e.g.
clobbers of address register to the check patterns in ia64.md _and_ allow
limited instructions (e.g. ones writing to general or fp regs) for BE_IN_SPEC
speculation in ia64.c, i.e. to be moved up past a speculation check.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

--- Comment #15 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-26 23:45:51 UTC ---
(In reply to comment #14)
>   So either we revert the patch at
> http://gcc.gnu.org/viewcvs?view=revision&revision=177658 or we add e.g.
> clobbers of address register to the check patterns in ia64.md _and_ allow
> limited instructions (e.g. ones writing to general or fp regs) for BE_IN_SPEC
> speculation in ia64.c, i.e. to be moved up past a speculation check.

I think reverting that patch is the right thing to do, iff it completely fixes
this problem (i.e. no similar issues lurking...). It may be worth adding a
FIXME note with a reference to this PR. Perhaps addressing this problem fully
isn't on anyone's TODO list for the moment (ia64...) but if speculation becomes
more important in future target then it's nice to know why things look the way
they do in the compiler.

The second option is more than minor surgery to the ia64 back end, and I think
that's more trouble than it's worth at this point. If a bug like this can go
undetected for such a long time (almost a year), then test coverage for the
ia64 back end is... well... not perfect ;-)

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[steven at gcc dot gnu.org]

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

--- Comment #16 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-26 23:52:32 UTC ---
Patch post for r177658: http://gcc.gnu.org/ml/gcc-patches/2011-08/msg00353.html

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

Andrey Belevantsev <abel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #17 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-07-30 11:01:35 UTC ---
I'm testing the revert of the below patch with a clarified comment.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

--- Comment #18 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-07-31 10:56:59 UTC ---
Author: abel
Date: Tue Jul 31 10:56:52 2012
New Revision: 190005

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190005
Log:
        PR target/53975

        * sel-sched-ir.c (has_dependence_note_reg_use): Clarify comment.

        Revert
        2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>

        * sel-sched-ir.c (has_dependence_note_reg_use): Call ds_full_merge
        only if producer writes to the register given by regno.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/sel-sched-ir.c

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

--- Comment #19 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-07-31 11:11:22 UTC ---
Fixed on trunk.  Judging by the time the original wrong patch went in, this
should be a regression and thus I'll commit this to 4.7 too after a week or so.

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

--- Comment #20 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-10-16 13:20:37 UTC ---
Author: abel
Date: Tue Oct 16 13:20:30 2012
New Revision: 192497

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192497
Log:
        Backport from mainline
        2012-07-31  Andrey Belevantsev  <abel@ispras.ru>
        PR target/53975

        * sel-sched-ir.c (has_dependence_note_reg_use): Clarify comment.
        Revert
        2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>
        * sel-sched-ir.c (has_dependence_note_reg_use): Call ds_full_merge
        only if producer writes to the register given by regno.


Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/sel-sched-ir.c

[Bug target/53975] [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction

[abel at gcc dot gnu.org]

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

Andrey Belevantsev <abel at gcc dot gnu.org> changed:

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

--- Comment #21 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-10-16 13:24:23 UTC ---
Fixed for trunk and 4.7.