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
