Bug 101674 - gcc.dg/uninit-pred-9_b.c fails after jump threading rewrite
Summary: gcc.dg/uninit-pred-9_b.c fails after jump threading rewrite
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wuninitialized
  Show dependency treegraph
 
Reported: 2021-07-29 10:30 UTC by Aldy Hernandez
Modified: 2023-04-11 07:18 UTC (History)
6 users (show)

See Also:
Host:
Target: powerpc64*-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-07-29 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aldy Hernandez 2021-07-29 10:30:48 UTC
The ranger-based jump threader (commit 2e96b5f14e4025691b57d2301d71aa6092ed44bc) shuffles things around in such a way as to confuse the -Wuninitialized pass.
Comment 1 Martin Sebor 2021-07-29 15:38:07 UTC
I can confirm the test fails (despite the xfail):

FAIL: gcc.dg/uninit-pred-9_b.c bogus warning (test for bogus messages, line 25)

The xfail target should probably be powerpc64*-*-*.  Since the failure is on line 25 would it make sense to xfail just that assertion?
Comment 2 Aldy Hernandez 2021-07-29 15:50:16 UTC
(In reply to Martin Sebor from comment #1)
> I can confirm the test fails (despite the xfail):
> 
> FAIL: gcc.dg/uninit-pred-9_b.c bogus warning (test for bogus messages, line
> 25)
> 
> The xfail target should probably be powerpc64*-*-*.  Since the failure is on
> line 25 would it make sense to xfail just that assertion?

Ahhh. Yes, that seems appropriate.  Feel free to commit it as obvious.  Thanks for taking care of this.
Comment 3 GCC Commits 2021-07-29 16:14:00 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:2f6bdd51cfe15403085b69c133065ebda4af9bb9

commit r12-2600-g2f6bdd51cfe15403085b69c133065ebda4af9bb9
Author: Martin Sebor <msebor@redhat.com>
Date:   Thu Jul 29 10:11:18 2021 -0600

    Xfail just the failing assertion and correct target.
    
    Related to:
    PR middle-end/101674 - gcc.dg/uninit-pred-9_b.c fails after jump threading rewrite
    
    gcc/testsuite:
            PR middle-end/101674
            * gcc.dg/uninit-pred-9_b.c: Xfail just the failing assertion and
            correct target.
Comment 4 GCC Commits 2021-07-31 00:32:09 UTC
The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:

https://gcc.gnu.org/g:309ddde04f2335f51062690328f03ce889be7e22

commit r12-2647-g309ddde04f2335f51062690328f03ce889be7e22
Author: Hans-Peter Nilsson <hp@bitrange.com>
Date:   Sat Jul 31 01:23:20 2021 +0200

    gcc.dg/uninit-pred-9_b.c: Xfail for MMIX too
    
    Looks like MMIX is the "correct target" too (cf. 2f6bdd51cfe15)
    and from
    https://gcc.gnu.org/pipermail/gcc-testresults/2021-July/710188.html
    it seems powerpc-ibm-aix7.2.3.0 is too, but I've not found
    other targets failing.
    
    gcc/testsuite:
            PR middle-end/101674
            * gcc.dg/uninit-pred-9_b.c: Xfail for mmix-*-* too.
Comment 5 GCC Commits 2021-08-11 00:37:06 UTC
The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:

https://gcc.gnu.org/g:92f7016940e5a7281e3fd7628fbf1360d900b581

commit r12-2843-g92f7016940e5a7281e3fd7628fbf1360d900b581
Author: Hans-Peter Nilsson <hp@axis.com>
Date:   Wed Aug 11 01:40:12 2021 +0200

    gcc.dg/uninit-pred-9_b.c: Xfail for CRIS too
    
    Adding to the growing list, for autotester accounting purposes.
    
    FWIW I see this fails for m68k too:
    https://gcc.gnu.org/pipermail/gcc-testresults/2021-August/712395.html
    and moxie:
    https://gcc.gnu.org/pipermail/gcc-testresults/2021-August/712389.html
    and pru:
    https://gcc.gnu.org/pipermail/gcc-testresults/2021-August/712366.html
    
    testsuite:
            PR middle-end/101674
            * gcc.dg/uninit-pred-9_b.c: Xfail for cris-*-* too.
Comment 6 sandra 2021-11-10 18:17:28 UTC
Also fails for nios2 (line 20, same as powerpc64).  Are we really just xfailing this everywhere instead of fixing the bug and/or testcase?  That kind of seems like sweeping the whole mess under the rug, to me.  :-S
Comment 7 Vineet Gupta 2022-05-24 01:30:19 UTC
Failing for riscv as well !
Comment 8 Richard Biener 2022-08-08 12:30:37 UTC
Bumping MAX_CHAIN_LEN to 7 removes warnings of it being exceeded during analysis but the diagnostic still happens.
Comment 9 Richard Biener 2022-08-22 13:41:51 UTC
uninit analysis has the guarding condition r_14(D) <= 18 but the guard of
the PHI is computed in non-optimal way since this is a PHI use of a PHI
with an uninitialized val on an edge but the paths we construct in
predicate::use_cannot_happen only consider the second PHI uninit edge.

bb 12:
# v_31 = PHI <v_30(D)(10), v_13(D)(11)>
...

bb 14:
# v_32 = PHI <v_31(23), r_14(D)(19)>
..

 = blah (v_32);

when we visit v_31 we determine an uninit use in v_32 on the 23->14 edge
and queue that PHI for analysis where we then find the blah (v_32) use.
But this queueing forgets that the paths that guard this uninit use
PHI def need all go through 12 -> 11.

The code in tree-ssa-uninit.cc does this, so the gimple-predicate-analysis.cc
code doesn't know anything about this.  That's a weakness in the API,
predicate::is_use_guarded (the API is somewhat awkward generally).

It might be we want the path from BB 12 through BB 14 to the use in the
'use_preds' or like suggested above in the def preds instead.  When
the put it into the 'use_preds' we get r_14(D) <= 18 && _7 != 0 && l_18(D) > 100
where we fail to open-code _7 (defined as _1 & _6).  At least we then
get the proper PHI def chain into the uninit use edge so such change alone
would be a correctness improvement here.

Now, we do pick up the definition of _1 & _6 but only after normalization
which we apply to the predicate of the PHI def but not to the predicate
of the use until after the use_cannot_happen test is complete.

But one issue is that normalization does nothing to !((_1 & _2) != 0)
which would be !(_1 != 0) | !(_2 != _0) but we don't do anything to that
in the chain normalization case (we'd need to duplicate here).  The
single pred case would also not do this because of the is_neq_zero_form_p
guard.

Not to say that the use_preds.use_cannot_happen case looks awfully similar
to the superset_of.  The main difference seems to be that init_from_phi_def
picks up a PHI def predicate starting only from the immediate dominator
while use_cannot_happen tries to build "fancy" predicates starting from
function entry.  And some use m_eval to compute 'opnds' while others
use the mask passed in to is_use_guarded.

The code is somewhat of a mess here.  It's all mightly complicated but
limited at the same time :/
Comment 10 Richard Biener 2022-09-01 12:56:53 UTC
For the remaining superset_of path we now have

        ((m_16(D) != 0) AND (r_18(D) <= 19) AND (m_16(D) != 100) AND (n_15(D) <= 9))
        OR ((l_22(D) > 100) AND (NOT (m_16(D) != 0)) AND (r_18(D) <= 19) AND (m_16(D) != 100) AND (n_15(D) <= 9))

guarding the well-definedness of

        v_59 = PHI <v_58(25), r_18(D)(21)>

and

       ((r_18(D) <= 18))

guarding the use.  The use predicate fails to prove either of the definition
guards which possibly is because the use predicate does not cover the path
from the upthread PHIs to the PHI in question:

<bb 13> [local count: 716722667]:
# v_58 = PHI <v_57(11), v_17(D)(12)>    // related PHI
if (l_22(D) > 100)
  goto <bb 14>; [50.00%]
else
  goto <bb 24>; [50.00%]

<bb 14> [local count: 358361334]:
_6 = m_16(D) <= 99;
_7 = _1 & _6;
if (_7 != 0)
  goto <bb 25>; [37.31%]
else
  goto <bb 26>; [62.69%]

<bb 15> [local count: 183623112]:
# v_59 = PHI <v_58(25), r_18(D)(21)>   // main PHI
if (r_18(D) <= 18)
  goto <bb 16>; [52.89%]
else
  goto <bb 27>; [47.11%]

<bb 16> [local count: 88583700]:
blah (v_59);

here the related PHI def v_58 is marked as possibly undefined so we use
the predicates from its incoming edges (and one more related PHI in BB11).
But the use predicate of v_58 in the v_59 PHI def isn't taken into
account.  That one would be

        ((m_16(D) <= 99) AND (n_15(D) <= 9) AND (l_22(D) > 100))

but that still looks insufficent when combined with (r_18(D) <= 18) to prove

((m_16(D) != 0) AND (r_18(D) <= 19) AND (m_16(D) != 100) AND (n_15(D) <= 9))

in particular the m_16(D) != 0 predicate.  I'm also seeing a missed
simplifcation of NOT (m_16(D) != 0) but that will not help either.