Re: Remove noce_mem_write_may_trap_or_fault_p in ifcvt

[Jeff Law <law@redhat.com>]

On 11/20/2015 07:08 AM, Bernd Schmidt wrote:
>
> BZ27313 is marked as fixed by the introduction of the tree cselim pass,
> thus the problem won't even be seen at RTL level.
Cool.

> 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.
I don't think cs-elim is safe WRT locks and such in multi-threaded code.

    In particular it replaces this:

      bb0:
        if (cond) goto bb2; else goto bb1;
      bb1:
        *p = RHS;
      bb2:

    with

      bb0:
        if (cond) goto bb1; else goto bb2;
      bb1:
        condtmp' = *p;
      bb2:
        condtmp = PHI <RHS, condtmp'>
        *p = condtmp;


If *p is a shared memory location, then there may be another writer.  If 
that writer happens to store something in that location after the load 
of *p, but before the store to *p, then that store will get lost in the 
transformed pseudo code.

That seems to introduce a data race.  Presumably one would call the 
original code ill-formed WRT the C11/C++11 memory model since the shared 
location is not marked as such.

I'm willing to consider this an independent problem.



>
> 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.
Cool.  It doesn't have to be super-scientific.  Good to see it didn't 
harm anything -- thanks for going the extra mile on this one.

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