Bug 81661 - [7 Regression] ICE in gimplify_modify_expr, at gimplify.c:5638
Summary: [7 Regression] ICE in gimplify_modify_expr, at gimplify.c:5638
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 7.0
: P2 normal
Target Milestone: 7.4
Assignee: Jakub Jelinek
URL:
Keywords: ice-on-valid-code
: 82539 (view as bug list)
Depends on: 84117
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-02 07:21 UTC by Martin Liška
Modified: 2018-03-03 20:35 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-08-02 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2017-08-02 07:21:35 UTC
Starting from r237064 I see ICE in:

$ cat ice.i
int a, b, c;
void d ()
{
  while (a + c > b)
    a--;
}

$ gcc ice.i -O3 -ftrapv
In function ‘d’:
cc1: internal compiler error: in gimplify_modify_expr, at gimplify.c:5638
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://bugs.opensuse.org/> for instructions.

It's with -mtune=generic -march=x86-64.

Thanks
Comment 1 Richard Biener 2017-08-02 07:47:02 UTC
niter analysis creates sth that gimplifies to control flow because trapping insns are involved.

Confirmed.
Comment 2 Jakub Jelinek 2017-08-02 16:56:54 UTC
So do we need some predicate that will tell us if it is safe to evaluate some generic expression using force_gimple_operand* and bail out if it isn't possible?
Even if gimplify_cond_expr tried harder around:
3865		  if (gimplify_ctxp->allow_rhs_cond_expr
3866		      /* If either branch has side effects or could trap, it can't be
3867			 evaluated unconditionally.  */
3868		      && !TREE_SIDE_EFFECTS (then_)
3869		      && !generic_expr_could_trap_p (then_)
3870		      && !TREE_SIDE_EFFECTS (else_)
3871		      && !generic_expr_could_trap_p (else_))
3872		    return gimplify_pure_cond_expr (expr_p, pre_p);
3873	
3874		  tmp = create_tmp_var (type, "iftmp");
and built for the case where gimplify_pure_cond_expr can't be used a SSA_NAME and a PHI if we're in SSA, I guess the vast majority of callers of force_gimple_operand* don't really expect that the sequence might need to be split into several basic blocks.
Comment 3 rguenther@suse.de 2017-08-03 07:11:52 UTC
On Wed, 2 Aug 2017, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81661
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> So do we need some predicate that will tell us if it is safe to evaluate some
> generic expression using force_gimple_operand* and bail out if it isn't
> possible?

Yes, see for example niter analysis expand_simple_operations which avoids
to turn GIMPLE into GENERIC if that might trap.  I suspect we're
looking at a similar case here, just needs to be tracked down...

> Even if gimplify_cond_expr tried harder around:
> 3865              if (gimplify_ctxp->allow_rhs_cond_expr
> 3866                  /* If either branch has side effects or could trap, it
> can't be
> 3867                     evaluated unconditionally.  */
> 3868                  && !TREE_SIDE_EFFECTS (then_)
> 3869                  && !generic_expr_could_trap_p (then_)
> 3870                  && !TREE_SIDE_EFFECTS (else_)
> 3871                  && !generic_expr_could_trap_p (else_))
> 3872                return gimplify_pure_cond_expr (expr_p, pre_p);
> 3873    
> 3874              tmp = create_tmp_var (type, "iftmp");
> and built for the case where gimplify_pure_cond_expr can't be used a SSA_NAME
> and a PHI if we're in SSA, I guess the vast majority of callers of
> force_gimple_operand* don't really expect that the sequence might need to be
> split into several basic blocks.

Yes, which is why we shouldn't really support this.

Of course the real issue is that we can't emit a non-trapping PLUS_EXPR
besides forcing unsigned arithmetic.

That said, this would be a whole lot easier to the middle-end if
we would have lowered -ftrapv operations and the actual arithmetic
were just wrapping or undefined...  which is AFAIK where we want to
go anyway.  Given we now have the overflow IFNs maybe we can try
lowering this at gimplification time?  (and clear flag_trapv
afterwards)
Comment 4 Martin Liška 2017-10-13 09:28:42 UTC
*** Bug 82539 has been marked as a duplicate of this bug. ***
Comment 5 Richard Biener 2017-10-26 14:04:01 UTC
One possibility is instead of using

 may_be_zero ? 0 : niter

as niter for code-generation use

 (may_be_zero ? 0 : 1) * niter

This avoids the gimplification issue.  We assemble this as

        call    __addvsi3
        movl    %eax, %esi
        subl    %ebp, %esi
        cmpl    %ebp, %eax
        setge   %al
        movzbl  %al, %eax
        imull   %eax, %esi

so it definitely is ugly (we're lacking any pattern matching on this
turning it back to a COND_EXPR).  And of course we're then unconditionally
emitting a possibly trapping expression into the IL.

For the case in question SCEV analysis knows the expression is executed
if the loop is entered (otherwise it would have rewritten it to unsigned
arithmetic).  Also the niter expression is usually similar to the
condition operators.

So I think a solution boils down to the consumer doing sth more intelligent
than building a COND_EXPR and later expecting to just code-gen that via
force_gimple_operand.  For the vectorizer this could mean a workaround
like storing niter away somewhere and using a new SSA name for the
niter part in the COND_EXPR and making sure to emit the definition for it
in the correct place.  Or going the multiplication way suggested above
for maybe trapping niter (note that tree_could_trap_p doesn't work
recursively, the gimplifier has generic_expr_could_trap_p for this).

Which also means a solution could be to simply not vectorize
loops with IV operations that might trap.
Comment 6 Andrew Pinski 2017-10-28 11:44:58 UTC
(In reply to Richard Biener from comment #5)
> One possibility is instead of using
> 
>  may_be_zero ? 0 : niter
> 
> as niter for code-generation use
> 
>  (may_be_zero ? 0 : 1) * niter
> 
> This avoids the gimplification issue.  We assemble this as
> 
>         call    __addvsi3
>         movl    %eax, %esi
>         subl    %ebp, %esi
>         cmpl    %ebp, %eax
>         setge   %al
>         movzbl  %al, %eax
>         imull   %eax, %esi
> 
> so it definitely is ugly (we're lacking any pattern matching on this
> turning it back to a COND_EXPR).

r250377 added a pattern which should have caught that ...
Comment 7 rguenther@suse.de 2017-10-30 10:51:50 UTC
On Sat, 28 Oct 2017, pinskia at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81661
> 
> --- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #5)
> > One possibility is instead of using
> > 
> >  may_be_zero ? 0 : niter
> > 
> > as niter for code-generation use
> > 
> >  (may_be_zero ? 0 : 1) * niter
> > 
> > This avoids the gimplification issue.  We assemble this as
> > 
> >         call    __addvsi3
> >         movl    %eax, %esi
> >         subl    %ebp, %esi
> >         cmpl    %ebp, %eax
> >         setge   %al
> >         movzbl  %al, %eax
> >         imull   %eax, %esi
> > 
> > so it definitely is ugly (we're lacking any pattern matching on this
> > turning it back to a COND_EXPR).
> 
> r250377 added a pattern which should have caught that ...

I suspect we're missing cond ? 0 : 1 -> (int) cond for it to
trigger.
Comment 8 Richard Biener 2018-01-25 08:26:05 UTC
GCC 7.3 is being released, adjusting target milestone.
Comment 9 Jakub Jelinek 2018-02-01 10:08:58 UTC
Author: jakub
Date: Thu Feb  1 10:08:26 2018
New Revision: 257284

URL: https://gcc.gnu.org/viewcvs?rev=257284&root=gcc&view=rev
Log:
	PR tree-optimization/81661
	PR tree-optimization/84117
	* tree-eh.h (rewrite_to_non_trapping_overflow): Declare.
	* tree-eh.c: Include gimplify.h.
	(find_trapping_overflow, replace_trapping_overflow,
	rewrite_to_non_trapping_overflow): New functions.
	* tree-vect-loop.c: Include tree-eh.h.
	(vect_get_loop_niters): Use rewrite_to_non_trapping_overflow.
	* tree-data-ref.c: Include tree-eh.h.
	(get_segment_min_max): Use rewrite_to_non_trapping_overflow.

	* gcc.dg/pr81661.c: New test.
	* gfortran.dg/pr84117.f90: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr81661.c
    trunk/gcc/testsuite/gfortran.dg/pr84117.f90
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-data-ref.c
    trunk/gcc/tree-eh.c
    trunk/gcc/tree-eh.h
    trunk/gcc/tree-vect-loop.c
Comment 10 Jakub Jelinek 2018-02-01 10:31:11 UTC
Fixed on the trunk so far.
Comment 11 Jakub Jelinek 2018-03-03 13:25:58 UTC
Author: jakub
Date: Sat Mar  3 13:25:27 2018
New Revision: 258183

URL: https://gcc.gnu.org/viewcvs?rev=258183&root=gcc&view=rev
Log:
	Backported from mainline
	2018-02-01  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/81661
	PR tree-optimization/84117
	* tree-eh.h (rewrite_to_non_trapping_overflow): Declare.
	* tree-eh.c: Include gimplify.h.
	(find_trapping_overflow, replace_trapping_overflow,
	rewrite_to_non_trapping_overflow): New functions.
	* tree-vect-loop.c: Include tree-eh.h.
	(vect_get_loop_niters): Use rewrite_to_non_trapping_overflow.

	* gcc.dg/pr81661.c: New test.
	* gfortran.dg/pr84117.f90: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr81661.c
    branches/gcc-7-branch/gcc/testsuite/gfortran.dg/pr84117.f90
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
    branches/gcc-7-branch/gcc/tree-eh.c
    branches/gcc-7-branch/gcc/tree-eh.h
    branches/gcc-7-branch/gcc/tree-vect-loop.c
Comment 12 Jakub Jelinek 2018-03-03 20:35:00 UTC
Fixed for 7.4+.