[Bug tree-optimization/18694] [4.0 regression] loop miscompilation at -O1 (-ftree-ch)

["law at redhat dot com" <gcc-bugzilla@gcc.gnu.org>]

------- Additional Comments From law at redhat dot com  2004-12-09 19:52 -------
Subject: Re:  [4.0 regression] loop
	miscompilation at -O1 (-ftree-ch)

On Thu, 2004-12-09 at 19:22 +0000, kazu at cs dot umass dot edu wrote:
> ------- Additional Comments From kazu at cs dot umass dot edu  2004-12-09 19:22 -------
> Jeff,
> 
> With my new patch, stmt.i gets one fewer jump threading opportunities
> (compared to what the vanilla mainline produces).
> So it's very plausible that we are miscompiling stmt.c quietly.
Certainly possible.

> 
> This difference comes from DOM1 while compiling stmt.c:expand_asm_operands.
> 
> I hope this helps.
It does.  I'm just now getting started and knowing the routine in 
question certainly narrows the search field.

> 
> With my old patch, I got one more jump threading opportunity
> (compared to what the vanilla mainline produces).
> It was one of loop.i, stmt.i, and predict.i.
> 
> Since my old patch gives less information to lookup routines,
> the increase in the number of opportunity seems very strange.
Not really if you know the history of this code :(

The selection code (thread_across_edge) dates from before the
SSA updating code could handle threading through a block with
side effects that needed to be preserved.

So the selection code would go through some "interesting" 
contortions to prove that statements in the block were
just nops.  So for example given

X_10 = <expr>

We would lookup <expr> in the hash tables.  If we got a hit
and the result was X_n, then we would consult the current
definition of X.  If the current definition of X was X_n,
then the statement above is really just

X_10 = X_n

Which is the same as

X = X

Which is a nop and thus can be ignored.


Now if we look at your change:

       tree src = PHI_ARG_DEF_FROM_EDGE (phi, e);
       tree dst = PHI_RESULT (phi);
+      if (TREE_CODE (src) == SSA_NAME
+	  && TREE_CODE (SSA_NAME_DEF_STMT (src)) == PHI_NODE
+	  && bb_for_stmt (SSA_NAME_DEF_STMT (src)) == e->dest)
+	continue;
       record_const_or_copy (dst, src);
       register_new_def (dst, &block_defs_stack);

We see that when the IF's condition holds we continue the loop, meaning
that we don't record a new definition for the LHS of the PHI -- which
means we don't update the current definition for the PHI_RESULT's
underlying variable.

The net result is that we have the wrong current definition for some
objects.  This in turn could lead to (incorrectly) threading a jump that
was not threadable before.

It may be enough to remove the "continue" and instead move the call
to record_const_or_copy into the IF statement's THEN clause.  
That prevents recording the bogus equivalence, but still keeps
the current definition of each variable up-to-date.

This is all speculation at this point.  I'm going to dig into this
code under the control of the debugger to see what's going on when
we compile stmt.c

> 
> If you want, I can find out which of DOM[123] is causing this problem
> on which function in which file for you.
No need.

I suspect it'll be pretty obvious given that we thread jumps differently
in loop.i, stmt.i and predict.i.

jeff




-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18694

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.