From patchwork Thu Oct 18 23:41:29 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [path] PR 54900: store data race in if-conversion pass From: Aldy Hernandez X-Patchwork-Id: 192481 Message-Id: <> To: Ian Lance Taylor Cc: Jakub Jelinek , gcc-patches , Richard Guenther , Ian Lance Taylor , Andrew MacLeod Date: Thu, 18 Oct 2012 19:41:29 -0400 On 10/16/12 20:33, Ian Lance Taylor wrote: > On Tue, Oct 16, 2012 at 4:54 PM, Aldy Hernandez wrote: >> On 10/16/12 13:28, Jakub Jelinek wrote: >>> location of considered if-conversion on mem >>> if (some_condition) >>> call (); >>> mem = something; >> >> >> Are we not currently being conservative in the current code and returning >> false once we see a volatile or a non-const call? > > Jakub's point is that in his example the call will not be a > post-dominator of the block. it's a good point. Would you mind > trying to create a test case along those lines? Ah, I see. Well, I can't come up with a testcase that fits into our testing infrastructure because of the aforementioned problems catching an addition of zero. I could scan the assembly, but anything I could come up with is not only back-end specific, but extremely fragile. I am attaching a testcase that I have used to (manually) test the patch below. The relevant parts are in the attached testcase is: ... TOP_BB outerfunc (innerfunc ()); if (flag_for_funky) funky(); dont_write += 666; My approach is, once we find the DONT_WRITE block, we generate a list of all the blocks at are post-dominated by it, and if there is a volatile/call that is between TOP_BB and the write, then a volatile/call will occur and we can't perform the speculation. This works with the testcase at hand, though probably a bit inefficient. For one, we could calculate the dominance info earlier in the pass, though I prefer calculating it only if we're actually considering speculating. I'm open to suggestions. I'm running into a weird verify_dominators() problem during bootstrap-- which is odd, because I'm forcing a recompute of the dominance tree. But I wanted to post this to make sure I'm at least poking in the right direction. Thoughts? PR rtl-optimization/54900 * ifcvt.c (volatile_or_non_const_call): New. (noce_can_store_speculate_p): Handle intervening calls. /* Compile with -O2. */ /* There shouldn't be a an unconditional write to dont_write in main(). */ extern int printf(const char *format, ...); int flag = 1; int flag_for_funky; int stuff; int *stuffp = &stuff; int dont_write; extern void funky(); void outerfunc (p1) { *stuffp = 0; } int innerfunc () { if (flag) return 0; ++dont_write; return 0; } void main () { outerfunc (innerfunc ()); if (flag_for_funky) funky(); dont_write += 666; } diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 5654c66..c615efd 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -2388,6 +2388,16 @@ noce_mem_write_may_trap_or_fault_p (const_rtx mem) return false; } +/* Return TRUE if INSN is a volatile insn or a non const call. */ + +static inline bool +volatile_or_non_const_call (rtx insn) +{ + return (INSN_P (insn) + && (volatile_insn_p (PATTERN (insn)) + || (CALL_P (insn) && (!RTL_CONST_CALL_P (insn))))); +} + /* Return whether we can use store speculation for MEM. TOP_BB is the basic block above the conditional block where we are considering doing the speculative store. We look for whether MEM is set @@ -2410,13 +2420,47 @@ noce_can_store_speculate_p (basic_block top_bb, const_rtx mem) have to stop looking. Even if the MEM is set later in the function, we still don't want to set it unconditionally before the barrier. */ - if (INSN_P (insn) - && (volatile_insn_p (PATTERN (insn)) - || (CALL_P (insn) && (!RTL_CONST_CALL_P (insn))))) + if (volatile_or_non_const_call (insn)) return false; + /* We can speculate if the MEM will be set on every path out + of TOP_BB, but we must be careful that there is no + intervening call/volatile between TOP_BB and the set. + + For the intervening call/volatile case, We are looking + for something akin to: + + location of considered if-conversion on mem + if (condition) + call (); + mem = something; */ if (memory_must_be_modified_in_insn_p (mem, insn)) - return true; + { + if (!dom_info_available_p (CDI_DOMINATORS)) + calculate_dominance_info (CDI_DOMINATORS); + + /* Get all blocks that are post-dominated by the SET. */ + VEC (basic_block, heap) *bbs = + get_all_dominated_blocks (CDI_POST_DOMINATORS, + BLOCK_FOR_INSN (insn)); + while (VEC_length (basic_block, bbs)) + { + rtx call; + basic_block bb = VEC_pop (basic_block, bbs); + + /* If there is an intervening volatile/call between + TOP_BB and the SET of MEM, bail. */ + FOR_BB_INSNS (bb, call) + if (volatile_or_non_const_call (call) + && dominated_by_p (CDI_DOMINATORS, + BLOCK_FOR_INSN (call), + top_bb)) + return false; + } + VEC_free (basic_block, heap, bbs); + return true; + } + if (modified_in_p (XEXP (mem, 0), insn)) return false;