[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #15 from Jakub Jelinek  ---
Author: jakub
Date: Thu Jan 14 15:25:22 2016
New Revision: 232368

URL: https://gcc.gnu.org/viewcvs?rev=232368=gcc=rev
Log:
PR middle-end/68146
PR tree-optimization/69155
* tree-complex.c: Include cfganal.h.
(phis_to_revisit): New variable.
(extract_component): Add phiarg_p argument.  Assert that returned
SSA_NAME has non-NULL SSA_NAME_DEF_STMT unless phiarg_p is true.
(update_phi_components): Partly rewrite to use loop over real/imag
components instead of code duplication.  If extract_component returns
SSA_NAME with NULL SSA_NAME_DEF_STMT, store SSA_NAME_VAR or
create_tmp_reg into the PHI node instead, and mention the phi triplet
in phis_to_revisit.
(tree_lower_complex): Walk bbs in rpo order.  Adjust phis recorded
in phis_to_revisit at the end.

* gfortran.dg/pr68146.f: New test.
* gfortran.dg/pr69155.f90: New test.

Added:
trunk/gcc/testsuite/gfortran.dg/pr68146.f
trunk/gcc/testsuite/gfortran.dg/pr69155.f90
Modified:
trunk/gcc/ChangeLog
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree-complex.c

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

Jakub Jelinek  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #16 from Jakub Jelinek  ---
Fixed.

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

Jakub Jelinek  changed:

   What|Removed |Added

  Attachment #37335|0   |1
is obsolete||
 Status|NEW |ASSIGNED
   Assignee|unassigned at gcc dot gnu.org  |jakub at gcc dot gnu.org

--- Comment #14 from Jakub Jelinek  ---
Created attachment 37340
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37340=edit
gcc6-pr69155.patch

Untested fix.

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P1

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #12 from Jakub Jelinek  ---
Created attachment 37335
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37335=edit
gcc6-pr69155-wip.patch

Partial patch I've bootstrapped/regtested on x86_64-linux and i686-linux. 
Either our testsuite coverage is insufficient, or it might be safe to only deal
with PHI arguments and nothing else if we walk in rpo order.
In that case, as the renamer is not prepared to handle PHIs with
non-is_gimple_val arguments and renaming them, it might be much easier to just
not use the renamer and simply record the PHIs we still need to update, push
the underlying VAR_DECLs (or extra temporaries for anon SSA_NAMEs) to the PHI
arguments first and get back to them at the end (or immediately once the
SSA_NAME is defined?).

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #13 from rguenther at suse dot de  ---
On January 13, 2016 7:08:28 PM GMT+01:00, "jakub at gcc dot gnu.org"
 wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155
>
>--- Comment #12 from Jakub Jelinek  ---
>Created attachment 37335
>  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37335=edit
>gcc6-pr69155-wip.patch
>
>Partial patch I've bootstrapped/regtested on x86_64-linux and
>i686-linux. 
>Either our testsuite coverage is insufficient, or it might be safe to
>only deal
>with PHI arguments and nothing else if we walk in rpo order.
>In that case, as the renamer is not prepared to handle PHIs with
>non-is_gimple_val arguments and renaming them, it might be much easier
>to just
>not use the renamer and simply record the PHIs we still need to update,
>push
>the underlying VAR_DECLs (or extra temporaries for anon SSA_NAMEs) to
>the PHI
>arguments first and get back to them at the end (or immediately once
>the
>SSA_NAME is defined?).

I think it's enough to deal with PHIs and keeping a worklist of unhandled
edge/PHI pairs would indeed work (and use decls as placeholder there).

Richard.

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #8 from rguenther at suse dot de  ---
On Mon, 11 Jan 2016, rsandifo at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155
> 
> --- Comment #7 from rsandifo at gcc dot gnu.org  
> ---
> (In reply to Richard Biener from comment #6)
> > (In reply to Jakub Jelinek from comment #4)
> > > (In reply to Richard Biener from comment #2)
> > > > I think we have a dup/related bug where we run into the issue that
> > > > tree-complex.c
> > > > wrecks SSA form during its rewrite.
> > > 
> > > That is indeed what happens, but what solution do you see other than
> > > avoiding all the simplifications/optimizations that follow ssa edges?
> > 
> > In the other bug we discussed to do the processing in proper (dominator)
> > order.  Richard, any progress on that?
> 
> Sorry for dropping the ball on this.  I'd hit exactly the problem that
> Jakub described and wasn't sure how to proceed.  Walking in dominator
> order or reserve postorder doesn't help because we still need to create
> scalar SSA_NAME replacements for complex phis.  You then have the same
> problem for the definitions of the phi in cases where an argument comes
> from a backedge.

Ok, I thought that doing dominator order would at least improve
things a bit here.

>  It seemed like we'd need to do one of:
> 
> - initially make the scalar definition a GIMPLE_NOP and replace it
>   with a phi later once we've decomposed all arguments

I think we visit the PHI always first, only its backedge arguments may
have unvisited defs (if we do proper dominator order walks).

> - initially make the scalar definition an empty phi and fill in its
>   arguments later
> 
> - initially make the scalar definition an empty phi and fill in
>   arguments as soon as their value is known.
> 
> None of them seemed very appealing.  Like Jakub says, I'm worried
> that any incomplete definitions are going to cause problems with
> code making invalid assumptions.
> 
> In some ways having a null DEF_STMT seems like the clearest
> indication of what's going on.

Well, there is another choice...

- do not keep SSA form for PHI args on backedges

that is, when we call get_component_ssa_name for a USE and do not have
it registered yet instead of creating a stray SSA name create
a decl and mark it for SSA renaming (and call update_ssa in the pass 
TODO).

Needs to do dominator walks to avoid too many renamings and adding
an arg to get_component_ssa_name to indicate whether we are asking
for a DEF or USE.

I think that's the cleanest approach (even if it might not be the
"fastest" given SSA renaming costs).

Richard.

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #9 from rsandifo at gcc dot gnu.org  
---
(In reply to rguent...@suse.de from comment #8)
> Well, there is another choice...
> 
> - do not keep SSA form for PHI args on backedges
> 
> that is, when we call get_component_ssa_name for a USE and do not have
> it registered yet instead of creating a stray SSA name create
> a decl and mark it for SSA renaming (and call update_ssa in the pass 
> TODO).
> 
> Needs to do dominator walks to avoid too many renamings and adding
> an arg to get_component_ssa_name to indicate whether we are asking
> for a DEF or USE.
> 
> I think that's the cleanest approach (even if it might not be the
> "fastest" given SSA renaming costs).

TBH it doesn't seem that clean to me.  The pass already creates correct
(at the point that the pass completes) SSA form even when there are
cycles, so it doesn't seem a good idea to make it punt on that case
just because of the intermediate state.  (And presumably force other
similar global update passes to do the same.)

Wouldn't it be better to have an acceptable way of representing
the intermediate state?  After all, we already check
name_registered_for_update_p in the fold routines, so creating
an SSA name whose definition is pending doesn't seem any worse.

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #10 from rguenther at suse dot de  ---
On Tue, 12 Jan 2016, rsandifo at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155
> 
> --- Comment #9 from rsandifo at gcc dot gnu.org  
> ---
> (In reply to rguent...@suse.de from comment #8)
> > Well, there is another choice...
> > 
> > - do not keep SSA form for PHI args on backedges
> > 
> > that is, when we call get_component_ssa_name for a USE and do not have
> > it registered yet instead of creating a stray SSA name create
> > a decl and mark it for SSA renaming (and call update_ssa in the pass 
> > TODO).
> > 
> > Needs to do dominator walks to avoid too many renamings and adding
> > an arg to get_component_ssa_name to indicate whether we are asking
> > for a DEF or USE.
> > 
> > I think that's the cleanest approach (even if it might not be the
> > "fastest" given SSA renaming costs).
> 
> TBH it doesn't seem that clean to me.  The pass already creates correct
> (at the point that the pass completes) SSA form even when there are
> cycles, so it doesn't seem a good idea to make it punt on that case
> just because of the intermediate state.  (And presumably force other
> similar global update passes to do the same.)

Well but then arguably the folding routine unconditionally looking
up SSA names or the pass doing any folding with that intermediate
state is broken.

> Wouldn't it be better to have an acceptable way of representing
> the intermediate state?  After all, we already check
> name_registered_for_update_p in the fold routines, so creating
> an SSA name whose definition is pending doesn't seem any worse.

Sure, but folding a stmt with a use of such SSA name is broken
(IMHO).

The name_registered_for_update_p is a similar wart - if the pass
has inconsistent SSA it should be able to ask the folders to not
look into SSA defs.  The match-and-simplify machinery has ways
to do that but the new predicates simply unconditionally lookup
defs :(  [and need the name_registered_for_update_p hack]

Say we'd have "inconsistent" state in that the def stmt is
not pending but wrong, in this case the folding may end up
generating a wrong result.  What kind of "inconsistencies"
do we expect the folders to handle?

Doing the SSA renaming thing keeps the IL consistent.

We could also temporarily use GIMPLE_NOP defs (but then
these kind of SSA names are expected to be default-defs as well)

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

Jeffrey A. Law  changed:

   What|Removed |Added

 CC||dcb314 at hotmail dot com

--- Comment #11 from Jeffrey A. Law  ---
*** Bug 68146 has been marked as a duplicate of this bug. ***

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #6 from Richard Biener  ---
(In reply to Jakub Jelinek from comment #4)
> (In reply to Richard Biener from comment #2)
> > I think we have a dup/related bug where we run into the issue that
> > tree-complex.c
> > wrecks SSA form during its rewrite.
> 
> That is indeed what happens, but what solution do you see other than
> avoiding all the simplifications/optimizations that follow ssa edges?

In the other bug we discussed to do the processing in proper (dominator)
order.  Richard, any progress on that?

> We have:
> :
> # a_22 = PHI 
> # b_23 = PHI 
> # a$real_28 = PHI 
> # a$imag_29 = PHI 
> # b$real_32 = PHI 
> # b$imag_33 = PHI 
> _9 = __complex__ (1.0e+0, 0.0) / a_22;
> b_10 = b_23 - _9;
> a_11 = a_22 + __complex__ (1.0e+0, 0.0);
> _8 = REALPART_EXPR ;
> if (_8 < 1.0e+1)
>   goto ;
> else
>   goto ;
> 
> While we could avoid the crash on lowering the division by handling it last,
> e.g. the addition has a loop of dependencies, so either we create a PHI
> first, but then don't have definitions for its arguments, or we lower the +
> first, but then we don't have definition for the PHI.  We can't create all
> the statements in a transaction together.
> Do you suggest we set some temporary SSA_NAME_DEF_STMTs for the SSA_NAMEs we
> create and we don't have a real definition yet (say GIMPLE_NOP)?

No, that's somewhat gross and I like to avoid that.

> Wouldn't some simplification attempt to optimize it as uninitialized?
> Or stop using gimple_build* and revert to constructing it using builders
> that don't actually fold anything or try to optimize anything, and when we
> have everything fold all newly added statements?

tree-complex.c doesn't use gimple_build but gimplify_build which simply
uses fold_buildN.  The issue is that the new enhanced predicates
unconditionally
walk SSA ... (even if the match-and-simplify machinery is instructed not to
do that which it doesn't for GENERIC anyway)

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

--- Comment #7 from rsandifo at gcc dot gnu.org  
---
(In reply to Richard Biener from comment #6)
> (In reply to Jakub Jelinek from comment #4)
> > (In reply to Richard Biener from comment #2)
> > > I think we have a dup/related bug where we run into the issue that
> > > tree-complex.c
> > > wrecks SSA form during its rewrite.
> > 
> > That is indeed what happens, but what solution do you see other than
> > avoiding all the simplifications/optimizations that follow ssa edges?
> 
> In the other bug we discussed to do the processing in proper (dominator)
> order.  Richard, any progress on that?

Sorry for dropping the ball on this.  I'd hit exactly the problem that
Jakub described and wasn't sure how to proceed.  Walking in dominator
order or reserve postorder doesn't help because we still need to create
scalar SSA_NAME replacements for complex phis.  You then have the same
problem for the definitions of the phi in cases where an argument comes
from a backedge.  It seemed like we'd need to do one of:

- initially make the scalar definition a GIMPLE_NOP and replace it
  with a phi later once we've decomposed all arguments

- initially make the scalar definition an empty phi and fill in its
  arguments later

- initially make the scalar definition an empty phi and fill in
  arguments as soon as their value is known.

None of them seemed very appealing.  Like Jakub says, I'm worried
that any incomplete definitions are going to cause problems with
code making invalid assumptions.

In some ways having a null DEF_STMT seems like the clearest
indication of what's going on.

[Bug tree-optimization/69155] [6 Regression] ICE (segfault in gimple_stmt_nonnegative_warnv_p)

[tkoenig at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69155

Thomas Koenig  changed:

   What|Removed |Added

  Component|fortran |tree-optimization

--- Comment #5 from Thomas Koenig  ---
Just for completeness' sake, here is a C testcase. Needs -fcx-fortran-rules
to fail.

#include 

double complex pr69155(double complex a, double complex b)
{
  if (cimag(a) < 10)
{
  while (creal(a) < 10)
{
  b = b - 1/a;
  a = a + 1;
}
}
  return a+b;
}

ig25@linux-fd1f:/tmp> gcc -fcx-fortran-rules -O c.c
c.c: In Funktion »pr69155«:
c.c:3:16: interner Compiler-Fehler: Speicherzugriffsfehler
 double complex pr69155(double complex a, double complex b)
^~~

0xb473bf crash_signal
../../trunk/gcc/toplev.c:334
0x8e8b15 gimple_stmt_nonnegative_warnv_p(gimple*, bool*, int)
../../trunk/gcc/gimple-fold.c:6260
...