From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13344 invoked by alias); 4 Sep 2015 15:19:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 13326 invoked by uid 89); 4 Sep 2015 15:19:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 04 Sep 2015 15:19:05 +0000 Received: from KLMAIL01.kl.imgtec.org (unknown [192.168.5.35]) by Websense Email Security Gateway with ESMTPS id 1532D8406AAC2; Fri, 4 Sep 2015 16:18:59 +0100 (IST) Received: from hhmail02.hh.imgtec.org (10.100.10.20) by KLMAIL01.kl.imgtec.org (192.168.5.35) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 4 Sep 2015 16:19:01 +0100 Received: from hhmail02.hh.imgtec.org ([::1]) by hhmail02.hh.imgtec.org ([::1]) with mapi id 14.03.0235.001; Fri, 4 Sep 2015 16:19:01 +0100 From: Robert Suchanek To: Matthew Fortune , "Catherine_Moore@mentor.com" CC: "gcc-patches@gcc.gnu.org" Subject: [PATCH] [MIPS] Fix wrong instruction in the delay slot Date: Fri, 04 Sep 2015 15:27:00 -0000 Message-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00361.txt.bz2 Hi, The attached test case that uses __builtin_unreachable in the default case in a switch statement triggers a situation where a wrong instruction is pla= ced in the delay slot by the eager delay slot filler. The issue should be reproducible with ToT compiler with -mips32r2 -G0 -mno-= abicalls -O2. It appears that a possibly related issue is already reported to Bugzilla (b= ug 51513) where the branch is not optimized away, leaving the compare and branch inst= ructions. It would also appear that this should be fixed at the tree level, however, = handling a special case when a branch is generated, pointing to 'nowhere' block, is = probably=20 more suitable. Without the fix, the generated assembly is following: ... addiu $2,$3,-3=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 sltu $4,$2,33=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 .set noreorder=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 .set nomacro=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 beq $4,$0,$L22=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 lw $4,%lo(mips_cm_is64)($4) .set macro=20=20=20=20 .set reorder=20=20 =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 sll $4,$2,2=20=20 ... The load instruction should not be placed in the delay slot as it is a part= of the taken branch with an undefined behaviour. The 'sll' was expected to be = in the slot. This is a series of unfortunate events that causes this to happen: 1. After the expansion, the 'nowhere' block is placed after the switch stat= ement and behaves as a fallthrough case with a diamond shape. 2. The block reordering pass randomly rearranges the nowhere block and it h= appens to be placed before the load instruction. 3. The eager delay slot filler firstly redirects jumps by skipping consecut= ive labels (ignoring barriers), pointing directly to the load. The analysis is doom= ed to failure at this stage as the own thread is analysed and the shift instruction is rejecte= d as it uses conflicting resources (most likely because $4 is referenced in the load)= but the opposite thread succeeds partly because the set and needed registers sets do not = intersect when the resources are being computed by mark_target_live_regs (). IMO, the fix is to recognize the empty basic block that has a code_label fo= llowed by a barrier (ignoring notes and debug_insns), forbid going beyond the barrier= if the empty block is found in skip_consecutive_labels () and first_active_target_insn (). The patch was cross tested on mips-img-linux-gnu and sparc-linux-gnu. Regards, Robert gcc/ * reorg.c (label_with_barrier_p): New function. (skip_consecutive_labels): Use it. Don't skip the label if an empty block is found. (first_active_target_insn): Likewise. Don't ignore the empty block when searching for the next active instruction. gcc/testsuite * gcc.target/mips/builtin-unreachable-bug-1.c: New test. --- gcc/reorg.c | 28 +++++++ .../gcc.target/mips/builtin-unreachable-bug-1.c | 90 ++++++++++++++++++= ++++ 2 files changed, 118 insertions(+) create mode 100644 gcc/testsuite/gcc.target/mips/builtin-unreachable-bug-1= .c diff --git a/gcc/reorg.c b/gcc/reorg.c index cdaa60c..269f666 100644 --- a/gcc/reorg.c +++ b/gcc/reorg.c @@ -145,6 +145,30 @@ along with GCC; see the file COPYING3. If not see These functions are now only used here in reorg.c, and have therefore been moved here to avoid inadvertent misuse elsewhere in the compiler. = */ =20 +/* Return true iff a LABEL is followed by a BARRIER. Ignore notes and deb= ug + instructions. */ + +static bool +label_with_barrier_p (rtx_insn *label) +{ + bool empty_bb =3D true; + + if (GET_CODE (label) !=3D CODE_LABEL) + empty_bb =3D false; + else + label =3D NEXT_INSN (label); + + while (!BARRIER_P (label) && empty_bb) + { + if (!(DEBUG_INSN_P (label) + || NOTE_P (label))) + empty_bb =3D false; + label =3D NEXT_INSN (label); + } + + return empty_bb; +} + /* Return the last label to mark the same position as LABEL. Return LABEL itself if it is null or any return rtx. */ =20 @@ -159,6 +183,8 @@ skip_consecutive_labels (rtx label_or_return) rtx_insn *label =3D as_a (label_or_return); =20 for (insn =3D label; insn !=3D 0 && !INSN_P (insn); insn =3D NEXT_INSN (= insn)) + if (LABEL_P (insn) && label_with_barrier_p (insn)) + break; if (LABEL_P (insn)) label =3D insn; =20 @@ -267,6 +293,8 @@ first_active_target_insn (rtx insn) { if (ANY_RETURN_P (insn)) return insn; + if (LABEL_P (insn) && label_with_barrier_p (as_a (insn))) + return NULL_RTX; return next_active_insn (as_a (insn)); } =20 diff --git a/gcc/testsuite/gcc.target/mips/builtin-unreachable-bug-1.c b/gc= c/testsuite/gcc.target/mips/builtin-unreachable-bug-1.c new file mode 100644 index 0000000..65eb9a3 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/builtin-unreachable-bug-1.c @@ -0,0 +1,90 @@ +/* { dg-options "-mcompact-branches=3Dnever -G0 -mno-abicalls" } */ +/* { dg-final { scan-assembler-not "beq\t\\\$4,\\\$0,\\\$L\[0-9\]+\n\tlw\t= \\\$4,%lo\\(mips_cm_is64\\)\\(\\\$4\\)" } } */ + +enum +{ + CPU_R4700, + CPU_R5000, + CPU_R5500, + CPU_NEVADA, + CPU_RM7000, + CPU_SR71000, + CPU_4KC, + CPU_4KEC, + CPU_4KSC, + CPU_24K, + CPU_34K, + CPU_1004K, + CPU_74K, + CPU_ALCHEMY, + CPU_PR4450, + CPU_BMIPS32, + CPU_BMIPS3300, + CPU_BMIPS5000, + CPU_JZRISC, + CPU_M14KC, + CPU_M14KEC, + CPU_INTERAPTIV, + CPU_P5600, + CPU_PROAPTIV, + CPU_1074K, + CPU_M5150, + CPU_I6400, + CPU_R3000, + CPU_5KC, + CPU_5KE, + CPU_20KC, + CPU_25KF, + CPU_SB1, + CPU_SB1A, + CPU_XLP, + CPU_QEMU_GENERIC +}; + +struct cpuinfo_mips +{ + long options; + int isa_level; +} cpu_data[1]; + +struct thread_info +{ + int cpu; +}; + +int a, b, c, d, e, mips_cm_is64; + +static int __attribute__ ((__cold__)) +mips_sc_probe_cm3 () +{ + struct thread_info *info; + struct cpuinfo_mips *c =3D &cpu_data[info->cpu]; + if (mips_cm_is64) + c->options =3D 0; + return 1; +} + +int +mips_sc_probe () +{ + struct cpuinfo_mips ci =3D cpu_data[c]; + if (mips_cm_is64) + __asm__("" ::: "memory"); + if (d) + return mips_sc_probe_cm3 (); + if (ci.isa_level) + switch (a) + { + case CPU_QEMU_GENERIC: + case CPU_I6400: + case CPU_R3000: + case CPU_NEVADA: + case CPU_BMIPS3300: + break; + default: + __builtin_unreachable (); + } + switch (a) + case CPU_R3000: + e =3D b; +} --=20 2.4.5