public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [path] PR 54900: store data race in if-conversion pass
@ 2012-10-15 12:53 Aldy Hernandez
  2012-10-15 14:25 ` Andrew MacLeod
  2012-10-16 17:32 ` Ian Lance Taylor
  0 siblings, 2 replies; 12+ messages in thread
From: Aldy Hernandez @ 2012-10-15 12:53 UTC (permalink / raw)
  To: gcc-patches, Richard Guenther, Ian Lance Taylor, Jakub Jelinek,
	Andrew MacLeod

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-10-18 23:41 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-15 12:53 [path] PR 54900: store data race in if-conversion pass Aldy Hernandez
2012-10-15 14:25 ` Andrew MacLeod
2012-10-16 17:32 ` Ian Lance Taylor
2012-10-16 17:33   ` Jakub Jelinek
2012-10-17  0:33     ` Aldy Hernandez
2012-10-17  0:54       ` Ian Lance Taylor
2012-10-19  1:41         ` Aldy Hernandez
2012-10-17  0:26   ` Aldy Hernandez
2012-10-17  0:54     ` Ian Lance Taylor
2012-10-17  4:54     ` Richard Henderson
2012-10-17  5:18       ` Jeff Law
2012-10-17 21:48         ` Aldy Hernandez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).