Re: Remove noce_mem_write_may_trap_or_fault_p in ifcvt

[Bernd Schmidt <bschmidt@redhat.com>]

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

Ok, so this is a thorny problem. I thought I had a solution, and I'll 
start by describing it, but in the end I think it doesn't actually work.

On 11/06/2015 10:09 PM, Jeff Law wrote:
> On 11/06/2015 12:30 PM, Bernd Schmidt wrote:
>> Well, I think if MEM_READONLY_P is insufficient (and I guess people
>> could cast away const - bother), then the current
>> noce_mem_write_may_trap_or_fault_p should be simplified to "return
>> true;" and eliminated.  We could try to special case stack accesses
>> within a known limit later on.
>>
>> Maybe it doesn't even matter given that we call noce_can_store_speculate
>> immediately after this test.
> may_trap_or_fault_p already has this kind of knowledge.  It just doesn't
> know if the memory is a read or a write.  Hence my suggestion we should
> fix may_trap_or_fault_p.

For the current issue I've come to the conclusion that this kind of 
analysis is irrelevant here (and that is not subject to the problem I'll 
describe later), because of the use of noce_can_store_speculate. See below.

> Ripping out noce_mem_write_may_trap_or_fault_p without fixing
> may_trap_or_fault_p introduces a latent code code generation issue.

I don't think so, actually. One safe option would be to rip it out and 
just stop transforming this case, but let's start by looking at the code 
just a bit further down, calling noce_can_store_speculate. This was 
added later than the code we're discussing, and it tries to verify that 
the same memory location will unconditionally be written to at a point 
later than the one we're trying to convert (but why aren't we testing 
for prior writes?). That should make any may_trap test unnecessary, and 
we can just simply delete the call to 
noce_mem_write_may_trap_or_fault_p. (We could leave such a test in as a 
compile-time focused early-out, but it may actually be too conservative. 
We may have an address which we can't prove safe in isolation, but if we 
know that we also have an unconditional store, it must be safe.)

So... I was about to propose the attached patch, which also fixes some 
oversights in the can_store_speculate path: we shouldn't allow autoinc 
addresses here. The added test in noce_can_store_speculate_p is not 
quite necessary given that the same one is also added to 
memory_must_be_modified_in_insn_p, but it avoids the need for an insn 
walk in cases where it isn't necessary. This bootstrapped and tested ok 
on x86_64-linux.

But then, I looked at noce_can_store_speculate again, and it seems 
broken. We walk over the post-dominators of the block, see if we find a 
store, and fail if something modifies the address:

   for (dominator = get_immediate_dominator (CDI_POST_DOMINATORS, top_bb);
        dominator != NULL;
        dominator = get_immediate_dominator (CDI_POST_DOMINATORS, 
dominator))
     {
       rtx_insn *insn;

       FOR_BB_INSNS (dominator, insn)
	{
[...]
	  if (modified_in_p (XEXP (mem, 0), insn))
	    return false;
	}

But what if the address is modified, but not in a post-dominator block? 
Let's say

  if (cond)
    *p = x; // this is the store we want to convert
  if (other_cond)
    p = some_pointer;
  else
    p = some_other_pointer;
  *p = y; // we'll see this store which postdominates the original
          // one, but not the modifications of p

So I think that algorithm doesn't work. My suggestion at this point 
would be to give up on converting stores entirely (deleting 
noce_can_store_speculate_p and noce_mem_write_may_trap_or_fault_p) until 
someone produces a reasonable scratchpad patch.


Bernd

[-- Attachment #2: noce-write.diff --]
[-- Type: text/x-patch, Size: 4584 bytes --]

	* alias.c (memory_must_be_modified_in_insn_p): Return false for
	addresses with side effects.
	* ifcvt.c (noce_mem_write_may_trap_or_fault_p): Delete function.
	(noce_can_store_speculate_p): Return false for addresses with side
	effects.
	(noce_process_if_block): Rely only on noce_can_store_speculate_p.

Index: gcc/alias.c
===================================================================
--- gcc/alias.c	(revision 230541)
+++ gcc/alias.c	(working copy)
@@ -2974,6 +2974,10 @@ memory_must_be_modified_in_insn_p (const
 {
   if (!INSN_P (insn))
     return false;
+  /* Our rtx_equal_p tests will not do the right thing in this case.  */
+  if (side_effects_p (XEXP (mem, 0)))
+    return false;
+
   insn = PATTERN (insn);
   if (GET_CODE (insn) == SET)
     return set_dest_equal_p (insn, mem);
@@ -2984,7 +2988,7 @@ memory_must_be_modified_in_insn_p (const
 	{
 	  rtx sub = XVECEXP (insn, 0, i);
 	  if (GET_CODE (sub) == SET
-	      &&  set_dest_equal_p (sub, mem))
+	      && set_dest_equal_p (sub, mem))
 	    return true;
 	}
     }
Index: gcc/ifcvt.c
===================================================================
--- gcc/ifcvt.c	(revision 230541)
+++ gcc/ifcvt.c	(working copy)
@@ -2919,59 +2919,6 @@ noce_operand_ok (const_rtx op)
   return ! may_trap_p (op);
 }
 
-/* Return true if a write into MEM may trap or fault.  */
-
-static bool
-noce_mem_write_may_trap_or_fault_p (const_rtx mem)
-{
-  rtx addr;
-
-  if (MEM_READONLY_P (mem))
-    return true;
-
-  if (may_trap_or_fault_p (mem))
-    return true;
-
-  addr = XEXP (mem, 0);
-
-  /* Call target hook to avoid the effects of -fpic etc....  */
-  addr = targetm.delegitimize_address (addr);
-
-  while (addr)
-    switch (GET_CODE (addr))
-      {
-      case CONST:
-      case PRE_DEC:
-      case PRE_INC:
-      case POST_DEC:
-      case POST_INC:
-      case POST_MODIFY:
-	addr = XEXP (addr, 0);
-	break;
-      case LO_SUM:
-      case PRE_MODIFY:
-	addr = XEXP (addr, 1);
-	break;
-      case PLUS:
-	if (CONST_INT_P (XEXP (addr, 1)))
-	  addr = XEXP (addr, 0);
-	else
-	  return false;
-	break;
-      case LABEL_REF:
-	return true;
-      case SYMBOL_REF:
-	if (SYMBOL_REF_DECL (addr)
-	    && decl_readonly_section (SYMBOL_REF_DECL (addr), 0))
-	  return true;
-	return false;
-      default:
-	return false;
-      }
-
-  return false;
-}
-
 /* 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
@@ -2982,6 +2929,9 @@ noce_can_store_speculate_p (basic_block
 {
   basic_block dominator;
 
+  if (side_effects_p (XEXP (mem, 0)))
+    return false;
+
   for (dominator = get_immediate_dominator (CDI_POST_DOMINATORS, top_bb);
        dominator != NULL;
        dominator = get_immediate_dominator (CDI_POST_DOMINATORS, dominator))
@@ -3540,26 +3490,21 @@ noce_process_if_block (struct noce_if_in
 
   if (!set_b && MEM_P (orig_x))
     {
-      /* Disallow the "if (...) x = a;" form (implicit "else x = x;")
-	 for optimizations if writing to x may trap or fault,
-	 i.e. it's a memory other than a static var or a stack slot,
-	 is misaligned on strict aligned machines or is read-only.  If
-	 x is a read-only memory, then the program is valid only if we
-	 avoid the store into it.  If there are stores on both the
-	 THEN and ELSE arms, then we can go ahead with the conversion;
-	 either the program is broken, or the condition is always
-	 false such that the other memory is selected.  */
-      if (noce_mem_write_may_trap_or_fault_p (orig_x))
-	return FALSE;
+      /* Here we have the "if (...) x = a;" form (with an implicit
+	 "else x = x;"), where x is a store to memory.
 
-      /* Avoid store speculation: given "if (...) x = a" where x is a
-	 MEM, we only want to do the store if x is always set
-	 somewhere in the function.  This avoids cases like
+	 We want to avoid store speculation: we only want to do the store
+	 if x is always set on a path leading to this point without an
+	 intervening memory barrier.  This avoids cases like
 	   if (pthread_mutex_trylock(mutex))
 	     ++global_variable;
 	 where we only want global_variable to be changed if the mutex
 	 is held.  FIXME: This should ideally be expressed directly in
-	 RTL somehow.  */
+	 RTL somehow.
+
+	 A success of this test implies that the access cannot trap, so
+	 we need not call may_trap_or_fault_p; doing so may even lose
+	 valid optimization opportunities.  */
       if (!noce_can_store_speculate_p (test_bb, orig_x))
 	return FALSE;
     }