From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8498 invoked by alias); 8 Aug 2014 15:17:36 -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 8486 invoked by uid 89); 8 Aug 2014 15:17:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Aug 2014 15:17:33 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1XFlv7-0004AC-OT from Tom_deVries@mentor.com ; Fri, 08 Aug 2014 08:17:29 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Fri, 8 Aug 2014 08:17:29 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Fri, 8 Aug 2014 16:17:27 +0100 Message-ID: <53E4EA03.90806@mentor.com> Date: Fri, 08 Aug 2014 15:17:00 -0000 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: Richard Biener CC: Steven Bosscher , "gcc- >> GCC Patches" , Andrew Pinski Subject: Re: Fix if-conversion pass for dead type-unsafe code References: <53E4B4DC.3030901@mentor.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------020802030802060105080706" X-SW-Source: 2014-08/txt/msg00903.txt.bz2 --------------020802030802060105080706 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 5028 On 08-08-14 13:36, Richard Biener wrote: > On Fri, 8 Aug 2014, Tom de Vries wrote: > >> Steven, >> >> this patch fixes: >> - PR62004 (the if-conversion pass part, the tail-merge part is still todo), >> and >> - PR62030. >> >> In both cases, a valid program with a dead type-unsafe access is transformed >> by the if-conversion pass into an invalid program with a live type-unsafe >> access. >> >> The transformation done by the if-conversion pass that suffers from this >> problem is if-merging, replacing the if-then-else with the if-block or the >> then then-block. >> >> The patch fixes this problem by detecting when the if-block and the then-block >> are treated differently by alias analysis, and preventing the optimization in >> that case. >> >> This part of the patch fixes PR62004. >> ... >> @@ -2583,7 +2631,7 @@ noce_process_if_block (struct noce_if_info *if_info) >> >> /* Look and see if A and B are really the same. Avoid creating silly >> cmove constructs that no one will fix up later. */ >> - if (rtx_equal_p (a, b)) >> + if (rtx_interchangeable_p (a, b)) >> { >> /* If we have an INSN_B, we don't have to create any new rtl. Just >> move the instruction that we already have. If we don't have an >> ... >> >> This part of the patch fixes PR62030: >> ... >> @@ -2517,7 +2565,7 @@ noce_process_if_block (struct noce_if_info *if_info) >> || BLOCK_FOR_INSN (insn_b) != BLOCK_FOR_INSN >> (if_info->cond_earliest) >> || !NONJUMP_INSN_P (insn_b) >> || (set_b = single_set (insn_b)) == NULL_RTX >> - || ! rtx_equal_p (x, SET_DEST (set_b)) >> + || ! rtx_interchangeable_p (x, SET_DEST (set_b)) >> || ! noce_operand_ok (SET_SRC (set_b)) >> || reg_overlap_mentioned_p (x, SET_SRC (set_b)) >> || modified_between_p (SET_SRC (set_b), insn_b, jump) >> ... >> >> I've added the other fixes after review of the if-conversion pass for the same >> problem, I hope this is complete (well, at least for the if-conversion pass. Hmm, I see now that in noce_try_cmove_arith a new mem is generated, and merging of (some of the) mem attributes is done, so that part doesn't need fixing for 4.8/4.9: ... /* If we're handling a memory for above, emit the load now. */ if (is_mem) { tmp = gen_rtx_MEM (GET_MODE (if_info->x), target); /* Copy over flags as appropriate. */ if (MEM_VOLATILE_P (if_info->a) || MEM_VOLATILE_P (if_info->b)) MEM_VOLATILE_P (tmp) = 1; if (MEM_ALIAS_SET (if_info->a) == MEM_ALIAS_SET (if_info->b)) set_mem_alias_set (tmp, MEM_ALIAS_SET (if_info->a)); set_mem_align (tmp, MIN (MEM_ALIGN (if_info->a), MEM_ALIGN (if_info->b))); gcc_assert (MEM_ADDR_SPACE (if_info->a) == MEM_ADDR_SPACE (if_info->b)); set_mem_addr_space (tmp, MEM_ADDR_SPACE (if_info->a)); noce_emit_move_insn (if_info->x, tmp); } ... Dropped in attached patch. >> I >> wonder if cross-jumping suffers from the same problem). >> Cross-jumping uses merge_memattrs. >> The PR62030 test-case fails with trunk on MIPS. The PR62004 testcase fails >> with 4.8 on x86_64. But I think the problem exists in trunk, 4.9 and 4.8, it's >> just hard to trigger. >> >> Bootstrapped and reg-tested on x86_64, trunk. No issue found other than >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62060 , which looks like a >> test-case issue. >> >> OK for trunk, 4.9, 4.8? > > Maybe instead of a new mem_alias_equal_p simply compare MEM_ATTRs > with mem_attrs_eq_p? I propose to fix it this way (as attached) on 4.8/4.9/trunk, and maybe do a more efficient handling on trunk as a follow-up patch. I'll put this through bootstrap/test again. > Note that > > + if (flag_strict_aliasing > + && MEM_ALIAS_SET (a) != MEM_ALIAS_SET (b)) > + return false; > > looks wrong as it doesn't treat ALIAS_SET_MEMORY_BARRIER specially. Ah, I see. > Also note that PRE already does sth similar with using exp_equiv_p > and passing 'true' for the 'for_gcse' argument. In fact > using exp_equiv_p would likely improve if-conversion if used > in place of rtx_equal_p? > Interesting. I've noticed btw that exp_equiv_p does not use mem_attrs_eq_p, but a mem-attrs pointer-equal comparison: ... /* Can't merge two expressions in different alias sets, since we can decide that the expression is transparent in a block when it isn't, due to it being set with the different alias set. Also, can't merge two expressions with different MEM_ATTRS. They could e.g. be two different entities allocated into the same space on the stack (see e.g. PR25130). In that case, the MEM addresses can be the same, even though the two MEMs are absolutely not equivalent. But because really all MEM attributes should be the same for equivalent MEMs, we just use the invariant that MEMs that have the same attributes share the same mem_attrs data structure. */ if (MEM_ATTRS (x) != MEM_ATTRS (y)) return 0; ... Thanks, - Tom --------------020802030802060105080706 Content-Type: text/x-patch; name="0001-Conservative-fix-for-PR62004-PR62030.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-Conservative-fix-for-PR62004-PR62030.patch" Content-length: 6642 2014-08-08 Tom de Vries PR rtl-optimization/62004 PR rtl-optimization/62030 * ifcvt.c (rtx_interchangeable_p): New function. (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p. * emit-rtl.c (mem_attrs_eq_p): Remove static. * emit-rtl.h (mem_attrs_eq_p): Declare. * gcc.dg/pr62004.c: New test. * gcc.dg/pr62030.c: Same. * gcc.target/mips/pr62030-octeon.c: Same. diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index b5278bf..1cac518 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -263,7 +263,7 @@ mem_attrs_htab_hash (const void *x) /* Return true if the given memory attributes are equal. */ -static bool +bool mem_attrs_eq_p (const struct mem_attrs *p, const struct mem_attrs *q) { return (p->alias == q->alias diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h index 7268090..9ccee07 100644 --- a/gcc/emit-rtl.h +++ b/gcc/emit-rtl.h @@ -69,6 +69,7 @@ extern void set_reg_attrs_for_parm (rtx, rtx); extern void set_reg_attrs_for_decl_rtl (tree t, rtx x); extern void adjust_reg_mode (rtx, enum machine_mode); extern int mem_expr_equal_p (const_tree, const_tree); +extern bool mem_attrs_eq_p (const struct mem_attrs *, const struct mem_attrs *); extern bool need_atomic_barrier_p (enum memmodel, bool); diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index e3353a5..dd6df5b 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -294,6 +294,28 @@ block_fallthru (basic_block bb) return (e) ? e->dest : NULL_BLOCK; } + +/* Return true if RTXs A and B can be safely interchanged. */ + +static bool +rtx_interchangeable_p (const_rtx a, const_rtx b) +{ + if (!rtx_equal_p (a, b)) + return false; + + if (GET_CODE (a) != MEM) + return true; + + /* A dead type-unsafe memory reference is legal, but a live type-unsafe memory + reference is not. Interchanging a dead type-unsafe memory reference with + a live type-safe one creates a live type-unsafe memory reference, in other + words, it makes the program illegal. + We check here conservatively whether the two memory references have equal + memory attributes. */ + + return mem_attrs_eq_p (get_mem_attrs (a), get_mem_attrs (b)); +} + /* Go through a bunch of insns, converting them to conditional execution format if possible. Return TRUE if all of the non-note @@ -1014,6 +1036,9 @@ noce_try_move (struct noce_if_info *if_info) || (rtx_equal_p (if_info->a, XEXP (cond, 1)) && rtx_equal_p (if_info->b, XEXP (cond, 0)))) { + if (!rtx_interchangeable_p (if_info->a, if_info->b)) + return FALSE; + y = (code == EQ) ? if_info->a : if_info->b; /* Avoid generating the move if the source is the destination. */ @@ -2483,7 +2508,7 @@ noce_process_if_block (struct noce_if_info *if_info) if (! insn_b || insn_b != last_active_insn (else_bb, FALSE) || (set_b = single_set (insn_b)) == NULL_RTX - || ! rtx_equal_p (x, SET_DEST (set_b))) + || ! rtx_interchangeable_p (x, SET_DEST (set_b))) return FALSE; } else @@ -2496,7 +2521,7 @@ noce_process_if_block (struct noce_if_info *if_info) || BLOCK_FOR_INSN (insn_b) != BLOCK_FOR_INSN (if_info->cond_earliest) || !NONJUMP_INSN_P (insn_b) || (set_b = single_set (insn_b)) == NULL_RTX - || ! rtx_equal_p (x, SET_DEST (set_b)) + || ! rtx_interchangeable_p (x, SET_DEST (set_b)) || ! noce_operand_ok (SET_SRC (set_b)) || reg_overlap_mentioned_p (x, SET_SRC (set_b)) || modified_between_p (SET_SRC (set_b), insn_b, jump) @@ -2562,7 +2587,7 @@ noce_process_if_block (struct noce_if_info *if_info) /* Look and see if A and B are really the same. Avoid creating silly cmove constructs that no one will fix up later. */ - if (rtx_equal_p (a, b)) + if (rtx_interchangeable_p (a, b)) { /* If we have an INSN_B, we don't have to create any new rtl. Just move the instruction that we already have. If we don't have an diff --git a/gcc/testsuite/gcc.dg/pr62004.c b/gcc/testsuite/gcc.dg/pr62004.c new file mode 100644 index 0000000..c994a41 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr62004.c @@ -0,0 +1,47 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fno-tree-tail-merge" } */ + +struct node +{ + struct node *next; + struct node *prev; +}; + +struct node node; + +struct head +{ + struct node *first; +}; + +struct head heads[5]; + +int k = 2; + +struct head *head = &heads[2]; + +int +main () +{ + struct node *p; + + node.next = (void*)0; + + node.prev = (void *)head; + + head->first = &node; + + struct node *n = head->first; + + struct head *h = &heads[k]; + + heads[2].first = n->next; + + if ((void*)n->prev == (void *)h) + p = h->first; + else + /* Dead tbaa-unsafe load from ((struct node *)&heads[2])->next. */ + p = n->prev->next; + + return !(p == (void*)0); +} diff --git a/gcc/testsuite/gcc.dg/pr62030.c b/gcc/testsuite/gcc.dg/pr62030.c new file mode 100644 index 0000000..b8baf93 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr62030.c @@ -0,0 +1,50 @@ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +extern void abort (void); + +struct node +{ + struct node *next; + struct node *prev; +}; + +struct node node; + +struct head +{ + struct node *first; +}; + +struct head heads[5]; + +int k = 2; + +struct head *head = &heads[2]; + +static int __attribute__((noinline)) +foo (void) +{ + node.prev = (void *)head; + head->first = &node; + + struct node *n = head->first; + struct head *h = &heads[k]; + struct node *next = n->next; + + if (n->prev == (void *)h) + h->first = next; + else + n->prev->next = next; + + n->next = h->first; + return n->next == &node; +} + +int +main (void) +{ + if (foo ()) + abort (); + return 0; +} diff --git a/gcc/testsuite/gcc.target/mips/pr62030-octeon.c b/gcc/testsuite/gcc.target/mips/pr62030-octeon.c new file mode 100644 index 0000000..5e3d3b3 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/pr62030-octeon.c @@ -0,0 +1,50 @@ +/* { dg-do run } */ +/* { dg-options "-march=octeon" } */ + +extern void abort (void); + +struct node +{ + struct node *next; + struct node *prev; +}; + +struct node node; + +struct head +{ + struct node *first; +}; + +struct head heads[5]; + +int k = 2; + +struct head *head = &heads[2]; + +static int __attribute__((noinline)) +foo (void) +{ + node.prev = (void *)head; + head->first = &node; + + struct node *n = head->first; + struct head *h = &heads[k]; + struct node *next = n->next; + + if (n->prev == (void *)h) + h->first = next; + else + n->prev->next = next; + + n->next = h->first; + return n->next == &node; +} + +int +main (void) +{ + if (foo ()) + abort (); + return 0; +} -- 1.9.1 --------------020802030802060105080706--