On 6/1/21 6:01 AM, Richard Biener wrote:
On Mon, 31 May 2021, Andrew MacLeod wrote:
On 5/28/21 11:25 AM, Richard Biener wrote:
This makes sure to perform final value replacement of constants
when we also are sure to propagate those, like in VRP. This avoids
spurious diagnostics when doing so only from within SCCP which can
leave unreachable loops in the IL triggering bogus diagnostics.
The choice is VRP because it already defers to SCEV for PHI nodes
so the following makes it analyze final values for loop exit PHIs.
To make that work it arranges for loop-closed SSA to be built only
after inserting ASSERT_EXPRs.
Bootstrapped and tested on x86_64-unknown-linux-gnu.
It does
FAIL: gcc.dg/ubsan/pr94423.c -O2 (internal compiler error)
where VRP somehow propagates abnormals in a bogus way.
OK, so I analyzed this some more and it results from the hunk moving
loop-closed SSA construction after ASSERT_EXPR insertion in
execute_vrp. The motivation for this is that we end up splitting
the loop exit edge when inserting the ASSERT_EXPR, creating
non-loop-closed SSA and thus fail to pick up the final value.
Now with swapping insert and loop-closed SSA build we get LC
SSA PHIs on an abnormal loop exit in the above testcase which
messes up assert expr removal which does
/* Propagate the RHS into every use of the LHS. For SSA names
also propagate abnormals as it merely restores the original
IL in this case (an replace_uses_by would assert). */
in remove_range_assertions, explicitely ignoring constraints around
abnormals. But since LC SSA PHIs remain we fail IL verification.
Note the LC SSA PHI is only required because we insert a SSA def
via the ASSERT_EXPR in the loop body. We can fix up the IL detail
by marking the ASSERT_EXPR source appropriately via
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 450926d5f9b..705e2489eb1 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -3809,6 +3809,8 @@ vrp_asserts::remove_range_assertions ()
FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
SET_USE (use_p, var);
+ if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs))
+ SSA_NAME_OCCURS_IN_ABNORMAL_PHI (var) = 1;
}
else
replace_uses_by (lhs, var);
but we also never get rid of a SSA_NAME_OCCURS_IN_ABNORMAL_PHI marking.
One option would be to keep the order as-is but fixup assert expr
insertion to update/honor loop-closed SSA. But then - how far are you
with removing all this ASSERT_EXPR stuff?
Thanks,
Richard.
I hope we can replace the VRP pass as well this release.. which will
then kill off all the need for the assert expr stuff. Aldy did some
preliminary comparisons with VRP on our branch about a month ago...
running a ranger EVRP pass right before vrp1 and vrp2 then seeing what
VRP got that we missed. and I believe we were doing pretty well, but
we havent had a chance to look at it in detail to see if there is
anything major we miss. Maybe he can comment on where we were. Once I
get the rest of the branch into trunk, we'll take another stab at the
comparison because what is going into trunk is more consistent than what
was on the branch.
Right now I only have 3 remaining bits to bring over .. relations,
application of equivalences, and recomputations. With any luck I'll get
them in this week. we can then do another run by of how we compare to VRP.
Andrew
