diff mbox

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

Message ID 507DF364.2010208@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Oct. 16, 2012, 11:53 p.m. UTC
> 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.
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.

Comments

Ian Lance Taylor Oct. 17, 2012, 12:26 a.m. UTC | #1
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
Richard Henderson Oct. 17, 2012, 1:51 a.m. UTC | #2
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~
Jeff Law Oct. 17, 2012, 3:21 a.m. UTC | #3
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
diff mbox

Patch

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);