Bug 53975 - [ia64] Target register of a speculative load moved to a branch register prior to the chk.s instruction
Summary: [ia64] Target register of a speculative load moved to a branch register prior...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.7.1
: P3 normal
Target Milestone: ---
Assignee: Andrey Belevantsev
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2012-07-15 19:51 UTC by Jakub Jermar
Modified: 2019-06-16 18:17 UTC (History)
6 users (show)

See Also:
Host:
Target: ia64
Build:
Known to work:
Known to fail: 4.7.0, 4.7.1, 4.8.0
Last reconfirmed: 2012-07-15 00:00:00


Attachments
Reproducible testcase (7.66 KB, application/x-gzip)
2012-07-16 21:25 UTC, Jakub Jermar
Details
Somewhat reduced, preprocessed test case (2.48 KB, text/x-csrc)
2012-07-18 23:13 UTC, Steven Bosscher
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jermar 2012-07-15 19:51:47 UTC
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.
Comment 1 Steven Bosscher 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?
Comment 2 Steven Bosscher 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.
Comment 3 Steven Bosscher 2012-07-15 21:17:34 UTC
What does the recovery code at 4404710 look like?
Comment 4 Steven Bosscher 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...
Comment 5 Jakub Jermar 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.
Comment 6 Jakub Jermar 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.
Comment 7 Jakub Jermar 2012-07-16 21:25:47 UTC
Created attachment 27805 [details]
Reproducible testcase

Here is a trimmed down reproducible testcase which exhibits the problem. The tarball contains a README file with instructions.
Comment 8 Steven Bosscher 2012-07-18 23:13:32 UTC
Created attachment 27827 [details]
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)
Comment 9 Steven Bosscher 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?
Comment 10 Alexander Monakov 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 :)
Comment 11 Jakub Jermar 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.
Comment 12 stevenb.gcc@gmail.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 ;-)
Comment 13 Andrey Belevantsev 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.
Comment 14 Andrey Belevantsev 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.
Comment 15 Steven Bosscher 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 ;-)
Comment 16 Steven Bosscher 2012-07-26 23:52:32 UTC
Patch post for r177658: http://gcc.gnu.org/ml/gcc-patches/2011-08/msg00353.html
Comment 17 Andrey Belevantsev 2012-07-30 11:01:35 UTC
I'm testing the revert of the below patch with a clarified comment.
Comment 18 Andrey Belevantsev 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
Comment 19 Andrey Belevantsev 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.
Comment 20 Andrey Belevantsev 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
Comment 21 Andrey Belevantsev 2012-10-16 13:24:23 UTC
Fixed for trunk and 4.7.