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

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

[Aldy Hernandez]

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

[Ian, I am copying you because this is originally your code.  Richard, I 
am copying you because you are all things aliased :-).  And Andrew is 
here because I am unable to come up with a suitable test for the 
simulate-thread harness.].

Here we have a store data race because noce_can_store_speculate_p() 
incorrectly returns true.  The problem is that 
memory_modified_in_insn_p() returns true if an instruction *MAY* modify 
a memory location, but the store can only be speculated if we are 
absolutely sure the store will happen on all subsequent paths.

My approach is to implement a memory_SURELY_modified_in_insn_p(), which 
will trigger this optimization less often, but at least it will be correct.

I thought of restricting the speculation for "--param 
allow-store-data-race=0" or transactional code, but IMO the speculation 
is incorrect as is.

I am having a bit of a problem coming up with a generic testcase. 
Perhaps Andrew or others have an idea.

The attached testcase fails to trigger without the patch, because AFAICT 
we have no way of testing an addition of zero to a memory location:

         cmpl    $1, flag(%rip)
         setb    %al
         addl    %eax, dont_write(%rip)

In the simulate-thread harness I can test the environment before an 
instruction, and after an instruction, but adding 0 to *dont_write 
produces no measurable effects, particularly in a back-end independent 
manner.  Ideas?

Bootstrap and regtested on x86-64 Linux.

Patch OK? (Except the test itself.)

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


	PR tree-optimization/54900
	* ifcvt.c (noce_can_store_speculate_p): Call
	memory_surely_modified_in_insn_p.
	* alias.c (memory_surely_modified_in_insn_p): New.
	(memory_modified_in_insn_p): Change comment.

diff --git a/gcc/alias.c b/gcc/alias.c
index 0c6a744..26d3797 100644
--- a/gcc/alias.c
+++ b/gcc/alias.c
@@ -2748,8 +2748,8 @@ memory_modified_1 (rtx x, const_rtx pat ATTRIBUTE_UNUSED, void *data)
 }
 
 
-/* Return true when INSN possibly modify memory contents of MEM
-   (i.e. address can be modified).  */
+/* Return true if INSN *MAY* possibly modify the memory contents of
+   MEM (i.e. address can be modified).  */
 bool
 memory_modified_in_insn_p (const_rtx mem, const_rtx insn)
 {
@@ -2760,6 +2760,22 @@ memory_modified_in_insn_p (const_rtx mem, const_rtx insn)
   return memory_modified;
 }
 
+/* Like memory_modified_in_insn_p, but return TRUE if INSN will
+   *SURELY* modify the memory contents of MEM.  */
+bool
+memory_surely_modified_in_insn_p (const_rtx mem, const_rtx insn)
+{
+  if (!INSN_P (insn))
+    return false;
+  rtx set = single_set (insn);
+  if (set)
+    {
+      rtx dest = SET_DEST (set);
+      return rtx_equal_p (dest, mem);
+    }
+  return false;
+}
+
 /* Initialize the aliasing machinery.  Initialize the REG_KNOWN_VALUE
    array.  */
 
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 2f486a2..659e464 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2415,7 +2415,7 @@ noce_can_store_speculate_p (basic_block top_bb, const_rtx mem)
 		  || (CALL_P (insn) && (!RTL_CONST_CALL_P (insn)))))
 	    return false;
 
-	  if (memory_modified_in_insn_p (mem, insn))
+	  if (memory_surely_modified_in_insn_p (mem, insn))
 	    return true;
 	  if (modified_in_p (XEXP (mem, 0), insn))
 	    return false;
diff --git a/gcc/rtl.h b/gcc/rtl.h
index cd5d435..d449ee1 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2614,6 +2614,7 @@ extern void init_alias_analysis (void);
 extern void end_alias_analysis (void);
 extern void vt_equate_reg_base_value (const_rtx, const_rtx);
 extern bool memory_modified_in_insn_p (const_rtx, const_rtx);
+extern bool memory_surely_modified_in_insn_p (const_rtx, const_rtx);
 extern bool may_be_sp_based_p (rtx);
 extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int);
 extern rtx get_reg_known_value (unsigned int);
diff --git a/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-5.c b/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-5.c
new file mode 100644
index 0000000..52daa27
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-5.c
@@ -0,0 +1,63 @@
+/* { dg-do link } */
+/* { dg-options "--param allow-store-data-races=0" } */
+/* { dg-final { simulate-thread } } */
+
+#include <stdio.h>
+#include "simulate-thread.h"
+
+/* PR tree-optimization/54900 */
+
+/* This file tests that a speculative store does not happen unless we
+   are sure a store to the location would happen anyway.  */
+
+int flag = 1;
+int stuff;
+int *stuffp = &stuff;
+int dont_write = 555;
+
+void simulate_thread_other_threads() 
+{
+}
+
+int simulate_thread_step_verify()
+{
+  if (dont_write != 555)
+    {
+      printf("FAIL: dont_write variable was assigned to.  \n");
+      return 1;
+    }
+  return 0;
+}
+
+int simulate_thread_final_verify()
+{
+  return 0;
+}
+
+void outerfunc (int p1)
+{
+  *stuffp = 0;
+}
+
+int innerfunc ()
+{
+  if (flag)
+    return 0;
+  /* This store should never happen because flag is globally set to true.  */
+  ++dont_write;
+  return 0;
+}
+
+__attribute__((noinline))
+void simulate_thread_main()
+{
+  outerfunc (innerfunc ());
+  simulate_thread_done();
+}
+
+__attribute__((noinline))
+int main()
+{
+  simulate_thread_main();
+  return 0;
+}

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

[Andrew MacLeod]

On 10/15/2012 08:28 AM, Aldy Hernandez wrote:
>
> I am having a bit of a problem coming up with a generic testcase. 
> Perhaps Andrew or others have an idea.
>
> The attached testcase fails to trigger without the patch, because 
> AFAICT we have no way of testing an addition of zero to a memory 
> location:
>
>         cmpl    $1, flag(%rip)
>         setb    %al
>         addl    %eax, dont_write(%rip)
>
> In the simulate-thread harness I can test the environment before an 
> instruction, and after an instruction, but adding 0 to *dont_write 
> produces no measurable effects, particularly in a back-end independent 
> manner.  Ideas?

Hum. isn't that clever.   Well, the instruction is executed pretty much 
atomically... so a write of the same value becomes very difficult to 
detect, and impossible within the existing harness. And I dont think a 
hardware watch point can catch that...

The only way I can think of is if you put 'dont_write' into a section 
which will trap if it is written to...  I don't know the details of 
doing such a thing or how you monitor the trap within the harness...

Other than that I'm not sure we can detect this with our current set of 
tools, for the longer term we'd need a write detector.  I don't suppose 
something like systemtap can detect writes like this?

Andrew

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

[Ian Lance Taylor]

On Mon, Oct 15, 2012 at 5:28 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Here we have a store data race because noce_can_store_speculate_p()
> incorrectly returns true.  The problem is that memory_modified_in_insn_p()
> returns true if an instruction *MAY* modify a memory location, but the store
> can only be speculated if we are absolutely sure the store will happen on
> all subsequent paths.
>
> My approach is to implement a memory_SURELY_modified_in_insn_p(), which will
> trigger this optimization less often, but at least it will be correct.

I don't see a reason to add this new function.  I would just inline it
into noce_can_store_speculate_p, replacing the call to
memory_modified_in_p.  And I'm not sure I see a reason to change the
comment for memory_modified_in_p, it seems to already be accurate.

Clearly we could consider the possibility of a PARALLEL of SET insns,
but of course most the compiler won't handle that anyhow.  I suppose
that would be a reason to use memory_surely_modified_in_insn_p, but in
that case you might as well handle the PARALLEL case now.

Ian

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

[Jakub Jelinek]

On Tue, Oct 16, 2012 at 10:19:54AM -0700, Ian Lance Taylor wrote:
> On Mon, Oct 15, 2012 at 5:28 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> I don't see a reason to add this new function.  I would just inline it
> into noce_can_store_speculate_p, replacing the call to
> memory_modified_in_p.  And I'm not sure I see a reason to change the
> comment for memory_modified_in_p, it seems to already be accurate.
> 
> Clearly we could consider the possibility of a PARALLEL of SET insns,
> but of course most the compiler won't handle that anyhow.  I suppose
> that would be a reason to use memory_surely_modified_in_insn_p, but in
> that case you might as well handle the PARALLEL case now.

I see another problem with noce_can_store_speculate_p, the return false;
checks.  The search for a store to exactly that location can be done
just on post dominator bbs, but I wonder how you can avoid looking for
volatile insns or non-const/pure calls (or insns that may modify
the address) also in other bbs.
What if there is:
  location of considered if-conversion on mem
  if (some_condition)
    call ();
  mem = something;
?  We only walk immediate post-dominators, so look at the bb containing
mem = something;, but I think a call/volatile insn/modification of mem's
address is problematic in any bb on any path in between test_bb and
the post-dominator on which the write has been found.

	Jakub

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

[Aldy Hernandez]

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


> I don't see a reason to add this new function.  I would just inline it
> into noce_can_store_speculate_p, replacing the call to
> memory_modified_in_p.  And I'm not sure I see a reason to change the
> comment for memory_modified_in_p, it seems to already be accurate.

My comment was just clearer to me, but probably because I wrote it :). 
I have revert it.

>
> Clearly we could consider the possibility of a PARALLEL of SET insns,
> but of course most the compiler won't handle that anyhow.  I suppose
> that would be a reason to use memory_surely_modified_in_insn_p, but in
> that case you might as well handle the PARALLEL case now.

Done.  Is this what you had in mind?

Tested on x86-64 Linux by looking at the generated assembly for the 
testcase with a dominating write and without.  Bootstrap and regtested 
as well.


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


	PR rtl-optimization/54900
	* ifcvt.c (noce_can_store_speculate_p): Call
	memory_surely_modified_in_insn_p.
	* alias.c (memory_surely_modified_in_insn_p): New.
	(set_dest_equal_p): New.

diff --git a/gcc/alias.c b/gcc/alias.c
index 0c6a744..f28e9a6 100644
--- a/gcc/alias.c
+++ b/gcc/alias.c
@@ -2760,6 +2760,39 @@ memory_modified_in_insn_p (const_rtx mem, const_rtx insn)
   return memory_modified;
 }
 
+/* Return TRUE if the destination of a set is rtx identical to
+   ITEM.  */
+static inline bool
+set_dest_equal_p (const_rtx set, const_rtx item)
+{
+  rtx dest = SET_DEST (set);
+  return rtx_equal_p (dest, item);
+}
+
+/* Like memory_modified_in_insn_p, but return TRUE if INSN will
+   *SURELY* modify the memory contents of MEM.  */
+bool
+memory_surely_modified_in_insn_p (const_rtx mem, const_rtx insn)
+{
+  if (!INSN_P (insn))
+    return false;
+  insn = PATTERN (insn);
+  if (GET_CODE (insn) == SET)
+    return set_dest_equal_p (insn, mem);
+  else if (GET_CODE (insn) == PARALLEL)
+    {
+      int i;
+      for (i = 0; i < XVECLEN (insn, 0); i++)
+	{
+	  rtx sub = XVECEXP (insn, 0, i);
+	  if (GET_CODE (sub) == SET
+	      &&  set_dest_equal_p (sub, mem))
+	    return true;
+	}
+    }
+  return false;
+}
+
 /* Initialize the aliasing machinery.  Initialize the REG_KNOWN_VALUE
    array.  */
 
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 2f486a2..659e464 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2415,7 +2415,7 @@ noce_can_store_speculate_p (basic_block top_bb, const_rtx mem)
 		  || (CALL_P (insn) && (!RTL_CONST_CALL_P (insn)))))
 	    return false;
 
-	  if (memory_modified_in_insn_p (mem, insn))
+	  if (memory_surely_modified_in_insn_p (mem, insn))
 	    return true;
 	  if (modified_in_p (XEXP (mem, 0), insn))
 	    return false;
diff --git a/gcc/rtl.h b/gcc/rtl.h
index cd5d435..d449ee1 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2614,6 +2614,7 @@ extern void init_alias_analysis (void);
 extern void end_alias_analysis (void);
 extern void vt_equate_reg_base_value (const_rtx, const_rtx);
 extern bool memory_modified_in_insn_p (const_rtx, const_rtx);
+extern bool memory_surely_modified_in_insn_p (const_rtx, const_rtx);
 extern bool may_be_sp_based_p (rtx);
 extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int);
 extern rtx get_reg_known_value (unsigned int);

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

[Aldy Hernandez]

On 10/16/12 13:28, Jakub Jelinek wrote:

> I see another problem with noce_can_store_speculate_p, the return false;
> checks.  The search for a store to exactly that location can be done
> just on post dominator bbs, but I wonder how you can avoid looking for
> volatile insns or non-const/pure calls (or insns that may modify
> the address) also in other bbs.
> What if there is:
>    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?

> ?  We only walk immediate post-dominators, so look at the bb containing
> mem = something;, but I think a call/volatile insn/modification of mem's
> address is problematic in any bb on any path in between test_bb and
> the post-dominator on which the write has been found.
>
> 	Jakub
>

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

[Ian Lance Taylor]

On Tue, Oct 16, 2012 at 4:53 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>
>> Clearly we could consider the possibility of a PARALLEL of SET insns,
>> but of course most the compiler won't handle that anyhow.  I suppose
>> that would be a reason to use memory_surely_modified_in_insn_p, but in
>> that case you might as well handle the PARALLEL case now.
>
>
> Done.  Is this what you had in mind?
>
> Tested on x86-64 Linux by looking at the generated assembly for the testcase
> with a dominating write and without.  Bootstrap and regtested as well.

This patch is OK with a ChangeLog entry.

Thanks.

Ian

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

[Ian Lance Taylor]

On Tue, Oct 16, 2012 at 4:54 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 10/16/12 13:28, Jakub Jelinek wrote:
>
>> I see another problem with noce_can_store_speculate_p, the return false;
>> checks.  The search for a store to exactly that location can be done
>> just on post dominator bbs, but I wonder how you can avoid looking for
>> volatile insns or non-const/pure calls (or insns that may modify
>> the address) also in other bbs.
>> What if there is:
>>    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?

Ian

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

[Richard Henderson]

On 2012-10-17 09:53, Aldy Hernandez wrote:
> +/* Like memory_modified_in_insn_p, but return TRUE if INSN will
> +   *SURELY* modify the memory contents of MEM.  */
> +bool
> +memory_surely_modified_in_insn_p (const_rtx mem, const_rtx insn)

I don't like the word "surely".  Are we certain or not?

It's longer, but perhaps "definitely" or "must_be"?


r~

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

[Jeff Law]

On 10/16/2012 07:51 PM, Richard Henderson wrote:
> On 2012-10-17 09:53, Aldy Hernandez wrote:
>> +/* Like memory_modified_in_insn_p, but return TRUE if INSN will
>> +   *SURELY* modify the memory contents of MEM.  */
>> +bool
>> +memory_surely_modified_in_insn_p (const_rtx mem, const_rtx insn)
>
> I don't like the word "surely".  Are we certain or not?
>
> It's longer, but perhaps "definitely" or "must_be"?
I'd go with "must_be" or something similar.  "must" is pretty common 
terminology when talking about aliasing properties.

jeff

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

[Aldy Hernandez]

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

On 10/16/12 23:21, Jeff Law wrote:
> On 10/16/2012 07:51 PM, Richard Henderson wrote:
>> On 2012-10-17 09:53, Aldy Hernandez wrote:
>>> +/* Like memory_modified_in_insn_p, but return TRUE if INSN will
>>> +   *SURELY* modify the memory contents of MEM.  */
>>> +bool
>>> +memory_surely_modified_in_insn_p (const_rtx mem, const_rtx insn)
>>
>> I don't like the word "surely".  Are we certain or not?
>>
>> It's longer, but perhaps "definitely" or "must_be"?
> I'd go with "must_be" or something similar.  "must" is pretty common
> terminology when talking about aliasing properties.
>
> jeff

must_be it is.

Committed the patch below.

Thanks.

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

    	PR rtl-optimization/54900
    	* ifcvt.c (noce_can_store_speculate_p): Call
    	memory_must_be_modified_in_insn_p.
    	* alias.c (memory_must_be_modified_in_insn_p): New.
    	(set_dest_equal_p): New.
    	* rtl.h (memory_must_be_modified_in_p): Protoize.
    
diff --git a/gcc/alias.c b/gcc/alias.c
index 244ca52..c5e6417 100644
--- a/gcc/alias.c
+++ b/gcc/alias.c
@@ -2762,6 +2762,39 @@ memory_modified_in_insn_p (const_rtx mem, const_rtx insn)
   return memory_modified;
 }
 
+/* Return TRUE if the destination of a set is rtx identical to
+   ITEM.  */
+static inline bool
+set_dest_equal_p (const_rtx set, const_rtx item)
+{
+  rtx dest = SET_DEST (set);
+  return rtx_equal_p (dest, item);
+}
+
+/* Like memory_modified_in_insn_p, but return TRUE if INSN will
+   *DEFINITELY* modify the memory contents of MEM.  */
+bool
+memory_must_be_modified_in_insn_p (const_rtx mem, const_rtx insn)
+{
+  if (!INSN_P (insn))
+    return false;
+  insn = PATTERN (insn);
+  if (GET_CODE (insn) == SET)
+    return set_dest_equal_p (insn, mem);
+  else if (GET_CODE (insn) == PARALLEL)
+    {
+      int i;
+      for (i = 0; i < XVECLEN (insn, 0); i++)
+	{
+	  rtx sub = XVECEXP (insn, 0, i);
+	  if (GET_CODE (sub) == SET
+	      &&  set_dest_equal_p (sub, mem))
+	    return true;
+	}
+    }
+  return false;
+}
+
 /* Initialize the aliasing machinery.  Initialize the REG_KNOWN_VALUE
    array.  */
 
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 2f486a2..5654c66 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2415,7 +2415,7 @@ noce_can_store_speculate_p (basic_block top_bb, const_rtx mem)
 		  || (CALL_P (insn) && (!RTL_CONST_CALL_P (insn)))))
 	    return false;
 
-	  if (memory_modified_in_insn_p (mem, insn))
+	  if (memory_must_be_modified_in_insn_p (mem, insn))
 	    return true;
 	  if (modified_in_p (XEXP (mem, 0), insn))
 	    return false;
diff --git a/gcc/rtl.h b/gcc/rtl.h
index eeeb6ba..09f1e77 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2616,6 +2616,7 @@ extern void init_alias_analysis (void);
 extern void end_alias_analysis (void);
 extern void vt_equate_reg_base_value (const_rtx, const_rtx);
 extern bool memory_modified_in_insn_p (const_rtx, const_rtx);
+extern bool memory_must_be_modified_in_insn_p (const_rtx, const_rtx);
 extern bool may_be_sp_based_p (rtx);
 extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int);
 extern rtx get_reg_known_value (unsigned int);

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

[Aldy Hernandez]

[-- 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;
}