[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 rsandifo at gcc dot gnu.org changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #24 from rsandifo at gcc dot gnu.org --- Yeah, bootstrap works again too. Thanks for the fix!
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #23 from Richard Biener --- This should fix the missed CSE Andrew noticed, not sure if it is enough to aovid the bogus diagnostic.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #22 from CVS Commits --- The master branch has been updated by Richard Biener : https://gcc.gnu.org/g:fe8475c500939011b90504304aec61bf6f48ac7d commit r12-4625-gfe8475c500939011b90504304aec61bf6f48ac7d Author: Richard Biener Date: Fri Oct 22 10:32:36 2021 +0200 bootstrap/102681 - properly CSE PHIs with default def args The PR shows that we fail to CSE PHIs containing (different) default definitions due to the fact on how we now handle on-demand build of VN_INFO. The following fixes this in the same way the PHI visitation code does. On gcc.dg/ubsan/pr81981.c this causes one expected warning to be elided since the uninit pass sees the change [local count: 1073741824]: # u$0_2 = PHI - # cstore_11 = PHI v = u$0_2; - return cstore_11; + return u$0_2; and thus only one of the conditionally uninitialized uses (the other became dead). I have XFAILed the missing diagnostic, I don't see a way to preserve that. 2021-10-22 Richard Biener PR bootstrap/102681 * tree-ssa-sccvn.c (vn_phi_insert): For undefined SSA args record VN_TOP. (vn_phi_lookup): Likewise. * gcc.dg/tree-ssa/ssa-fre-97.c: New testcase. * gcc.dg/ubsan/pr81981.c: XFAIL one case.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681
--- Comment #21 from Richard Biener ---
(In reply to Andrew Pinski from comment #14)
> Created attachment 51650 [details]
> Little more reduced
>
> So FRE is able to figure out for the following:
> # _20 = PHI <0(2), 1(3)>
> # const_upper_26 = PHI
>
> # _30 = PHI <0(12), 1(13)>
> # const_upper_33 = PHI
>
> That _30 is the same as _20 but not _26 is the same as _33 even though it
> does figure out that _19 and _29 are the same as _10. If it is able to
> figure that out, then things would just work.
>
> Richi,
> I assume FRE does not Value number default SSA names (non-parm) the same
> which is why this is happening is that correct?
The issue with CSE here is that with RPO VN I made unvisited vars
VARYING due to on-demand handling. While vn_visit_phis has special
handling for undefs the hashtable insert/lookup do not.
I am testing the following to rectify this (which then CSEs this PHI).
diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c
index ae0172a143e..893b1d0ddaa 100644
--- a/gcc/tree-ssa-sccvn.c
+++ b/gcc/tree-ssa-sccvn.c
@@ -4499,7 +4499,12 @@ vn_phi_lookup (gimple *phi, bool backedges_varying_p)
tree def = PHI_ARG_DEF_FROM_EDGE (phi, e);
if (TREE_CODE (def) == SSA_NAME
&& (!backedges_varying_p || !(e->flags & EDGE_DFS_BACK)))
- def = SSA_VAL (def);
+ {
+ if (ssa_undefined_value_p (def, false))
+ def = VN_TOP;
+ else
+ def = SSA_VAL (def);
+ }
vp1->phiargs[e->dest_idx] = def;
}
vp1->type = TREE_TYPE (gimple_phi_result (phi));
@@ -4543,7 +4548,12 @@ vn_phi_insert (gimple *phi, tree result, bool
backedges_varying_p)
tree def = PHI_ARG_DEF_FROM_EDGE (phi, e);
if (TREE_CODE (def) == SSA_NAME
&& (!backedges_varying_p || !(e->flags & EDGE_DFS_BACK)))
- def = SSA_VAL (def);
+ {
+ if (ssa_undefined_value_p (def, false))
+ def = VN_TOP;
+ else
+ def = SSA_VAL (def);
+ }
vp1->phiargs[e->dest_idx] = def;
}
vp1->value_id = VN_INFO (result)->value_id;
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #20 from Aldy Hernandez --- (In reply to Andrew Pinski from comment #16) > (In reply to Andrew Pinski from comment #15) > > We totally missed the jump threading of 3->5->7 path and the 4->5->8 path. > > FAIL: path through PHI in bb8 (incoming bb:6) crosses loop > > But but, it does not exactly cross the loop as 5 (6) is not part of the loop > but rather just 8. I looked at the IL, and this still counts as crossing loop boundaries. Your going from no loop to loop 1. No cigar :-(. Actually, even if you remove the hunk I mentioned the shared registry restrictions will disallow it. That being said, I should clean that up.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681
--- Comment #19 from Aldy Hernandez ---
(In reply to Andrew Pinski from comment #16)
> (In reply to Andrew Pinski from comment #15)
> > We totally missed the jump threading of 3->5->7 path and the 4->5->8 path.
>
> FAIL: path through PHI in bb8 (incoming bb:6) crosses loop
>
> But but, it does not exactly cross the loop as 5 (6) is not part of the loop
> but rather just 8.
Interesting. The restriction that tickles this is old legacy code in place
from way before I touched any of this:
// This is like path_crosses_loops in profitable_path_p but more
// restrictive, since profitable_path_p allows threading the
// first block because it would be redirected anyhow.
//
// If we loosened the restriction and used profitable_path_p()
// here instead, we would peel off the first iterations of loops
// in places like tree-ssa/pr14341.c.
bool profitable_p = m_path[0]->loop_father == e->src->loop_father;
if (!profitable_p && 0)
{
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file,
" FAIL: path through PHI in bb%d (incoming bb:%d) crosses
loop\n",
e->dest->index, e->src->index);
continue;
}
I even annotated it because it seemed strange that it was more restrictive than
the generic restrictions in the backward threader.
It is very possible that we can remove this, as we have much more thorough loop
restrictions in place in the shared registry.
If you remove the above chunk, does it work? If so, I may have to test and
benchmark the change.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #18 from Aldy Hernandez --- (In reply to Andrew Pinski from comment #9) > So in uninit1 we have: > if (_6691 != 0) > goto ; [5.50%] > else > goto ; [94.50%] > >[local count: 17344687]: > goto ; [100.00%] > >[local count: 298013267]: > >[local count: 315357954]: > # const_upper_3854 = PHI <_6687(87), 18446744073709551615(287)> > # _870 = PHI <1(87), 0(287)> [snip] > But _870 is _6691 == 0 but that relationship is totally missed and there is > full on jump threading miss in the above IR. This matches what I mentioned in the mailing list. If we could notice this relationship, we could thread over the uninitialized use.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 Tamar Christina changed: What|Removed |Added CC|tamar.christina at arm dot com | --- Comment #17 from Tamar Christina --- remove duplicate email
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #16 from Andrew Pinski --- (In reply to Andrew Pinski from comment #15) > We totally missed the jump threading of 3->5->7 path and the 4->5->8 path. FAIL: path through PHI in bb8 (incoming bb:6) crosses loop But but, it does not exactly cross the loop as 5 (6) is not part of the loop but rather just 8.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #15 from Andrew Pinski --- So the major difference comes from mark_stack_region_used. We have a missing jump thread in ethread. Before the patch, ethread was able to jump thread all the way through: if (_13 != 0) goto ; [5.50%] else goto ; [94.50%] : # _22 = PHI <0(2)> goto ; [INV] : # _18 = PHI <1(2)> _15 = upper_bound.coeffs[0]; goto ; [100.00%] : But after we get: : _13 = upper_bound.coeffs[1]; if (_13 != 0) goto ; [5.50%] else goto ; [94.50%] : # _22 = PHI <0(2)> goto ; [100.00%] : # _9 = PHI <1(2)> _15 = upper_bound.coeffs[0]; : # _16 = PHI <0(3), 1(4)> # const_upper_20 = PHI if (_16 != 0) goto ; [INV] else goto ; [INV] We totally missed the jump threading of 3->5->7 path and the 4->5->8 path. Aldy, Can you look into why there is a missing jump threading there?
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 Andrew Pinski changed: What|Removed |Added Attachment #51649|0 |1 is obsolete|| --- Comment #14 from Andrew Pinski --- Created attachment 51650 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51650=edit Little more reduced So FRE is able to figure out for the following: # _20 = PHI <0(2), 1(3)> # const_upper_26 = PHI # _30 = PHI <0(12), 1(13)> # const_upper_33 = PHI That _30 is the same as _20 but not _26 is the same as _33 even though it does figure out that _19 and _29 are the same as _10. If it is able to figure that out, then things would just work. Richi, I assume FRE does not Value number default SSA names (non-parm) the same which is why this is happening is that correct?
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 Andrew Pinski changed: What|Removed |Added Attachment #51648|0 |1 is obsolete|| --- Comment #13 from Andrew Pinski --- Created attachment 51649 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51649=edit Reduced testcase Reduced testcast attached, 68 lines. Which should be easier to figure out what is going on.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #12 from Andrew Pinski --- So this is definitely a bad interaction between complete unrolling where we had: for (unsigned int i = 1; i < 2; i++) if (this->coeffs[1] != 0) return false; And jump threading. I am still reducing the testcase but at least I figured out this part of it.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #11 from Andrew Pinski --- Good news I can reproduce the warning with the preprocessed source on a native x86_64-linux-gnu trunk GCC.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #10 from Andrew Pinski --- Hmm, somehow unroll messes up the relationship ...
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #9 from Andrew Pinski --- So in uninit1 we have: if (_6691 != 0) goto ; [5.50%] else goto ; [94.50%] [local count: 17344687]: goto ; [100.00%] [local count: 298013267]: [local count: 315357954]: # const_upper_3854 = PHI <_6687(87), 18446744073709551615(287)> # _870 = PHI <1(87), 0(287)> (lots of stuff) if (_6691 != 0) goto ; [5.50%] else goto ; [94.50%] [local count: 298013268]: goto ; [100.00%] [local count: 17344687]: [local count: 315357954]: # const_upper_3931 = PHI if (_870 != 0) goto ; [50.00%] else goto ; [50.00%] [local count: 157678977]: if (const_upper_3931 > _6695) goto ; [89.00%] else goto ; [11.00%] But _870 is _6691 == 0 but that relationship is totally missed and there is full on jump threading miss in the above IR.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #8 from Andrew Pinski --- Created attachment 51648 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51648=edit preprocessed source unreduced preprocessed source which fails still as of r12-4600. -fno-PIE -c -g -O2 -fno-checking -gtoggle -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-error=format-diag -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 --- Comment #7 from Andrew Pinski --- This still fails as of r12-4600-gf5ef4da3ccdfbedb . I will go debug this tomorrow to see what exactly is going on and why the warning is still not resolved.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 Andrew Pinski changed: What|Removed |Added See Also||https://gcc.gnu.org/bugzill ||a/show_bug.cgi?id=102794 --- Comment #6 from Andrew Pinski --- I have not looked into the IR here but what could be happening is jump threading is happening through the loop header and uninitialized code is getting so confused. The uninitialized code with respect to conditionals is so fragile with respect to jump thread; there are other bugs which show that.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681
rsandifo at gcc dot gnu.org changed:
What|Removed |Added
CC||rsandifo at gcc dot gnu.org
--- Comment #5 from rsandifo at gcc dot gnu.org
---
(In reply to Martin Sebor from comment #3)
> Simply initializing the variable as in the patch below avoids the warning.
> The control flow in the code is sufficiently opaque to make it worthwhile
> from a readability point irrespective of whether or not the variable can, in
> fact, be used uninitialized.
>
> index e50d3fc3b62..c7f0a405ff6 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -199,7 +199,7 @@ stack_region_maybe_used_p (poly_uint64 lower_bound,
> poly_uint64 upper_bound,
> static void
> mark_stack_region_used (poly_uint64 lower_bound, poly_uint64 upper_bound)
> {
> - unsigned HOST_WIDE_INT const_lower, const_upper;
> + unsigned HOST_WIDE_INT const_lower, const_upper = 0;
>const_lower = constant_lower_bound (lower_bound);
>if (upper_bound.is_constant (_upper))
> for (unsigned HOST_WIDE_INT i = const_lower; i < const_upper; ++i)
I disagree that this is better for readability. Initialising to zero
implies that the zero can reach code dominated by is_constant returning
true. In other words, it implies that the zero might be used and that
initialising to a different value would give different behaviour,
which in turn raises the question why 0 is the right choice.
The control flow for is_constant is just:
if (is_constant ())
{
*const_value = this->coeffs[0];
return true;
}
return false;
where it is clear that const_value is assigned to iff the
function returns true. If we can't handle this kind of
construct then I think we should try to punt on it.
It doesn't seem all that different from what would happen
for std::optional> after SRA, where AIUI
the second array element would be uninitialised if
_M_engaged is false.
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681
--- Comment #4 from Feng Xue ---
(In reply to Martin Sebor from comment #3)
> Simply initializing the variable as in the patch below avoids the warning.
> The control flow in the code is sufficiently opaque to make it worthwhile
> from a readability point irrespective of whether or not the variable can, in
> fact, be used uninitialized.
>
> index e50d3fc3b62..c7f0a405ff6 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -199,7 +199,7 @@ stack_region_maybe_used_p (poly_uint64 lower_bound,
> poly_uint64 upper_bound,
> static void
> mark_stack_region_used (poly_uint64 lower_bound, poly_uint64 upper_bound)
> {
> - unsigned HOST_WIDE_INT const_lower, const_upper;
> + unsigned HOST_WIDE_INT const_lower, const_upper = 0;
>const_lower = constant_lower_bound (lower_bound);
>if (upper_bound.is_constant (_upper))
> for (unsigned HOST_WIDE_INT i = const_lower; i < const_upper; ++i)
This code looks good, the warning seems to be an over-kill.
Will this change be checked in as a fix?
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681
--- Comment #3 from Martin Sebor ---
Simply initializing the variable as in the patch below avoids the warning. The
control flow in the code is sufficiently opaque to make it worthwhile from a
readability point irrespective of whether or not the variable can, in fact, be
used uninitialized.
index e50d3fc3b62..c7f0a405ff6 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -199,7 +199,7 @@ stack_region_maybe_used_p (poly_uint64 lower_bound,
poly_uint64 upper_bound,
static void
mark_stack_region_used (poly_uint64 lower_bound, poly_uint64 upper_bound)
{
- unsigned HOST_WIDE_INT const_lower, const_upper;
+ unsigned HOST_WIDE_INT const_lower, const_upper = 0;
const_lower = constant_lower_bound (lower_bound);
if (upper_bound.is_constant (_upper))
for (unsigned HOST_WIDE_INT i = const_lower; i < const_upper; ++i)
[Bug bootstrap/102681] [12 Regression] AArch64 bootstrap failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102681 Richard Biener changed: What|Removed |Added Component|tree-optimization |bootstrap Summary|AArch64 bootstrap failure |[12 Regression] AArch64 ||bootstrap failure Priority|P3 |P1 Target Milestone|--- |12.0 Version|unknown |12.0 Status|UNCONFIRMED |NEW Ever confirmed|0 |1 Keywords||build, diagnostic Target||aarch64
