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