Re: Remove noce_mem_write_may_trap_or_fault_p in ifcvt

[Jeff Law <law@redhat.com>]

On 11/18/2015 12:16 PM, Bernd Schmidt wrote:
>
> 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.
Hmm....

>
>> 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
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.

https://gcc.gnu.org/ml/gcc-patches/2007-08/msg01978.html

Sadly, no testcase was included.


> (but why aren't we testing
> for prior writes?).
Oversight I suspect.  Essentially you want to know if the store is 
anticipated (occurs on all paths to a given point).  There's a similar 
term for always occurs on all paths leaving a given point, but I can't 
remember it offhand.

>
> 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.
Right.....

>
> 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:
Egad.  That's clearly wrong.  Post domination means that paths to the 
exit must go through the post-dominator.  A path not


>
>    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
Right.  You essentially have to check all the blocks in the path.  At 
which point you might as well just run standard dataflow algorithms 
rather than coding up something ad-hoc here.

>
> 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.
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.


jeff