Re: [path] PR 54900: store data race in if-conversion pass

[Aldy Hernandez <aldyh@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On 10/16/12 20:33, Ian Lance Taylor wrote:
> On Tue, Oct 16, 2012 at 4:54 PM, Aldy Hernandez <aldyh@redhat.com> 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?

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 2548 bytes --]


	PR rtl-optimization/54900
	* ifcvt.c (volatile_or_non_const_call): New.
	(noce_can_store_speculate_p): Handle intervening calls.

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;
 

[-- Attachment #3: testcase.c --]
[-- Type: text/plain, Size: 471 bytes --]

/* 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;
}