Re: Remove noce_mem_write_may_trap_or_fault_p in ifcvt

[Bernd Schmidt <bschmidt at redhat dot com>]

On 11/19/2015 12:49 AM, Jeff Law wrote:
> On 11/18/2015 12:16 PM, Bernd Schmidt wrote:
> > 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
> And if we dig into that thread, Ian suggests this isn't terribly
> important anyway.
>
> However, if we go even further upthread, we find an assertion from
> Michael that this is critical for 456.hmmer and references BZ 27313.

Cc'd in case he has additional input.

> https://gcc.gnu.org/ml/gcc-patches/2007-08/msg01978.html
>
> Sadly, no testcase was included.

BZ27313 is marked as fixed by the introduction of the tree cselim pass, 
thus the problem won't even be seen at RTL level.
I'm undecided on whether cs-elim is safe wrt the store speculation vs 
locks concerns raised in the thread discussing Ian's 
noce_can_store_speculate_p, but that's not something we have to consider 
to solve the problem at hand.

> So if it weren't for the assertion that it's critical for hmmr, I'd be
> convinced that just ripping out was the right thing to do.
>
> Can you look at 27313 and hmmr and see if there's an impact.  Just maybe
> the critical stuff for those is handled by the tree if converter and we
> can just rip out the clearly incorrect RTL bits without regressing
> anything performance-wise.  If there is an impact, then I think we have
> to look at either improving the tree bits (so we can remove the rtl
> bits) or we have to do real dataflow analysis in the rtl bits.

So I made this change:

   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;
-
-      /* 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
-          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.  */
-      if (!noce_can_store_speculate_p (test_bb, orig_x))
-       return FALSE;
-    }
+    return FALSE;

As far as I can tell hmmer and the 27313 testcase are unaffected at -O2 
(if anything, hmmer was very slightly faster afterwards). The run wasn't 
super-scientific, but I wouldn't have expected anything else given the 
existence of cs-elim.

Ok to do the above, removing all the bits made unnecessary (including 
memory_must_be_modified_in_insn_p in alias.c)?


Bernd