[Bug c/99122] New: [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[Bug c/99122] New: [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[gscfq@t-online.de]

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

            Bug ID: 99122
           Summary: [10/11 Regression] ICE in force_constant_size, at
                    gimplify.c:733
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gscfq@t-online.de
  Target Milestone: ---

Changed between 20191103 and 20191110 at -O2+ :


$ cat z1.c
void f ()
{
  int n = 5;
  struct { char a[n]; } x;
  g (x, x);
}
int g (double x, double y)
{
  return __builtin_isgreater (x, y);
}


$ gcc-11-20210214 -c z1.c -O2
z1.c: In function 'f':
z1.c:5:3: warning: implicit declaration of function 'g'
[-Wimplicit-function-declaration]
    5 |   g (x, x);
      |   ^
during GIMPLE pass: einline
z1.c:5:3: internal compiler error: in force_constant_size, at gimplify.c:733
    5 |   g (x, x);
      |   ^~~~~~~~
0xa2e116 force_constant_size
        ../../gcc/gimplify.c:733
0xa361a7 gimple_add_tmp_var(tree_node*)
        ../../gcc/gimplify.c:771
0x9fbeea create_tmp_var(tree_node*, char const*)
        ../../gcc/gimple-expr.c:485
0xa4310f create_tmp_from_val
        ../../gcc/gimplify.c:564
0xa4310f lookup_tmp_var
        ../../gcc/gimplify.c:596
0xa4310f internal_get_tmp_var
        ../../gcc/gimplify.c:635
0xa3d7cb gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
        ../../gcc/gimplify.c:15058
0xa4404c gimplify_compound_lval
        ../../gcc/gimplify.c:3074
0xa3b115 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
        ../../gcc/gimplify.c:14022
0xa6a999 gimple_regimplify_operands(gimple*, gimple_stmt_iterator*)
        ../../gcc/gimplify-me.c:235
0xdcf752 insert_init_stmt
        ../../gcc/tree-inline.c:3397
0xddbbe9 setup_one_parameter
        ../../gcc/tree-inline.c:3568
0xde1e05 initialize_inlined_parameters
        ../../gcc/tree-inline.c:3595
0xde1e05 expand_call_inline
        ../../gcc/tree-inline.c:4956
0xde4d19 gimple_expand_calls_inline
        ../../gcc/tree-inline.c:5288
0xde4d19 optimize_inline_calls(tree_node*)
        ../../gcc/tree-inline.c:5461
0x185e393 early_inliner(function*)
        ../../gcc/ipa-inline.c:3007

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-02-16
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
          Component|c                           |ipa
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |jamborm at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
   Target Milestone|---                         |10.3

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r10-4444-g7313607478c11e9455a32fb0dbfd7867e04ea96a

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Another, still undefined, but perhaps slightly less so, testcase is:
static int foo ();

int
bar (int n)
{
  struct S { char a[n]; } x;
  __builtin_memset (x.a, 0, n);
  return foo (n, x);
}

static inline int
foo (int n, struct T { char a[n]; } b)
{
  int r = 0, i;
  for (i = 0; i < n; i++)
    r += b.a[i];
  return r;
}

I wonder if the easiest fix isn't just to disable inlining if any of the
arguments has variable length.  The inliner seems to be clearly unprepared to
handle that.  VLAs when passed are passed as pointers to the elements, so it is
only about variable length structures or if some front-end would pass arrays as
arrays and not as pointers.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #3 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Looking at how expr.c deals with WITH_SIZE_EXPR, perhaps we should do something
like the following:

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index a710fa59027..cdabeb6bafd 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3418,6 +3418,9 @@ force_value_to_type (tree type, tree value)
      Still if we end up with truly mismatched types here, fall back
      to using a VIEW_CONVERT_EXPR or a literal zero to not leak invalid
      GIMPLE to the following passes.  */
+  if (TREE_CODE (value) == WITH_SIZE_EXPR)
+    value = TREE_OPERAND (value, 0);
+
   if (!is_gimple_reg_type (TREE_TYPE (value))
           || TYPE_SIZE (type) == TYPE_SIZE (TREE_TYPE (value)))
     return fold_build1 (VIEW_CONVERT_EXPR, type, value);

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
With the patch from comment #3, the following sequence with the problematic
call:

  x.1_26 = __builtin_alloca_with_align (_24, 8);
  g (WITH_SIZE_EXPR <*x.1_26, _22>, WITH_SIZE_EXPR <*x.1_26, _22>);
  __builtin_stack_restore (saved_stack.2_15);

is turned by einline into:

  x.1_26 = __builtin_alloca_with_align (_24, 8);
  x_27 = MEM <double> [(struct  *)x.1_26];
  y_29 = MEM <double> [(struct  *)x.1_26];
  _30 = x_27 u<= y_29;
  _31 = ~_30;
  _32 = (int) _31;
  __builtin_stack_restore (saved_stack.2_15);

Which looks OKish, under the circumstances, to me.  Let me look at the second
testcase now.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That could perhaps work for the #c0 testcase where the function actually has a
non-VL parameter and so garbage in garbage out.
But would that work also for #c2?
If one dumps the #c2 testcase with -O2 -fno-inline -fdump-tree-optimized,
the PARM_DECL is clearly variable length but not lowered to *ptr, while
in the caller it is lowered that way and allocated through
__builtin_alloca_with_align.
So, clearly PARM_DECLs can be variable length but VAR_DECLs should not be (they
should be gimplified into ptr = __builtin_alloca_with_align with stack
save/restore around the scope and DECL_VALUE_EXPR of *ptr.
The inliner certainly doesn't do that right now.

For punting on inlining these, I couldn't find any spot that would try to
verify at least remote compatibility of the passed in arguments and the
arguments the callee expects.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #6 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> That could perhaps work for the #c0 testcase where the function actually has
> a non-VL parameter and so garbage in garbage out.
> But would that work also for #c2?

No, at least not on its own.  It leaks struct S, which is local to
bar, to the IL of foo, which hits an assert in make_decl_rtl.  Einline
dump has:

  struct T b;
  ...
  x.1_28 = __builtin_alloca_with_align (_26, 8);
  _20 = (long unsigned int) n_14(D);
  _21 = &x.1_28->a;
  __builtin_memset (_21, 0, _20);
  b = MEM <struct T> [(struct S *)x.1_28];

> If one dumps the #c2 testcase with -O2 -fno-inline -fdump-tree-optimized,
> the PARM_DECL is clearly variable length but not lowered to *ptr, while
> in the caller it is lowered that way and allocated through
> __builtin_alloca_with_align.
> So, clearly PARM_DECLs can be variable length but VAR_DECLs should not be
> (they should be gimplified into ptr = __builtin_alloca_with_align with stack
> save/restore around the scope and DECL_VALUE_EXPR of *ptr.
> The inliner certainly doesn't do that right now.
>
> For punting on inlining these, I couldn't find any spot that would try to
> verify at least remote compatibility of the passed in arguments and the
> arguments the callee expects.

No, with LTO it would be too late even if we tried to, (IPA) inlining
decisions are not meant to be un-doable.  The idea was that mismatches
are undefined and so we should try our best to emulate non-inlining
and not ICE.  But apparently we don't manage that now.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #6)
> > For punting on inlining these, I couldn't find any spot that would try to
> > verify at least remote compatibility of the passed in arguments and the
> > arguments the callee expects.
> 
> No, with LTO it would be too late even if we tried to, (IPA) inlining
> decisions are not meant to be un-doable.  The idea was that mismatches
> are undefined and so we should try our best to emulate non-inlining
> and not ICE.  But apparently we don't manage that now.

Well, I meant decide not to inline because either any of the gimple_call_arg
(stmt, ?) is a variable length or any of the expected arguments of the function
are variable length during the IPA inlining decisions.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
           Priority|P3                          |P2

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
I pushed us in this direction, so let me have a look.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> Another, still undefined, but perhaps slightly less so, testcase is:
> static int foo ();
> 
> int
> bar (int n)
> {
>   struct S { char a[n]; } x;
>   __builtin_memset (x.a, 0, n);
>   return foo (n, x);
> }
> 
> static inline int
> foo (int n, struct T { char a[n]; } b)
> {
>   int r = 0, i;
>   for (i = 0; i < n; i++)
>     r += b.a[i];
>   return r;
> }
> 
> I wonder if the easiest fix isn't just to disable inlining if any of the
> arguments has variable length.  The inliner seems to be clearly unprepared
> to handle that.  VLAs when passed are passed as pointers to the elements, so
> it is only about variable length structures or if some front-end would pass
> arrays as arrays and not as pointers.

So with fixing the original and this case by simply not trying to be clever
when setting up the parameter on mismatches like with

@@ -3418,7 +3421,9 @@ force_value_to_type (tree type, tree value)
      Still if we end up with truly mismatched types here, fall back
      to using a VIEW_CONVERT_EXPR or a literal zero to not leak invalid
      GIMPLE to the following passes.  */
-  if (!is_gimple_reg_type (TREE_TYPE (value))
+  if (TREE_CODE (value) == WITH_SIZE_EXPR)
+    return error_mark_node;
+  else if (!is_gimple_reg_type (TREE_TYPE (value))
           || TYPE_SIZE (type) == TYPE_SIZE (TREE_TYPE (value)))
     return fold_build1 (VIEW_CONVERT_EXPR, type, value);
   else

the ICEs are gone but the comment#2 testcase has a local var added
which cannot be assembled because we do

  /* Make an equivalent VAR_DECL.  Note that we must NOT remap the type
     here since the type of this decl must be visible to the calling
     function.  */
  var = copy_decl_to_var (p, id);

and obviously the sizes are not set up correctly for the caller.  That's
one complication - we have to massage the WITH_SIZE_EXPR size back to
the TYPE because the VLA bounds are not passed explicitely.  In a nested
function context those are transfered via the static chain so it should
work there but there the C FE marks the functions as not inlineable
(but IIRC for the unsupported return value handling).

Which makes me go back to IPA summary analysis, looking for the place
we can disable inlining of a certain cgraph edge based on call stmts
using WITH_SIZE_EXPR arguments.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Like with

diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index e32e69cd3ad..ac85be741b1 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -2775,7 +2775,12 @@ analyze_function_body (struct cgraph_node *node, bool
early)
                             (gimple_call_arg (stmt, i));
                    }
                }
-
+             for (unsigned int i = 0; i < gimple_call_num_args (stmt); ++i)
+               if (TREE_CODE (gimple_call_arg (stmt, i)) == WITH_SIZE_EXPR)
+                 {
+                   edge->inline_failed = CIF_FUNCTION_NOT_INLINABLE;
+                   break;
+                 }
              es->call_stmt_size = this_size;
              es->call_stmt_time = this_time;
              es->loop_depth = bb_loop_depth (bb);

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And one case is not covered, callee like foo in #c2, but caller passing non-VLA
argument (whatever, a double, struct S { char a[4]; }, int, etc.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
E.g.
static int foo ();

int
bar (int n)
{
  return foo (n, 2.0);
}

static inline int
foo (int n, struct T { char a[n]; } b)
{
  int r = 0, i;
  for (i = 0; i < n; i++)
    r += b.a[i];
  return r;
}

ICEs at -O2 too.  And there is no WITH_SIZE_EXPR argument in that case.
So I think next to looking for WITH_SIZE_EXPR arguments it should also look
through callee DECL_ARGUMENTS if any argument has non-constant TYPE_SIZE* (or
DECL_SIZE*?).

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #13 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I think that you want to disable inlining in the case when the callee has a
formal parameter which is a VLA (as opposed to a VLA actual argument of a
call), probably in inline_forbidden_p.  When just the caller does stupid
things, IMHO it can be fixed with the patch in comment #3.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #14 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Like with the following, which seems to work as far as inlining is concerned,
but the latest Jakub's example ICEs when cloning for IPA-CP :-/  (I am also not
sure if the predicate to identify VLAs is the best one).

--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3418,6 +3418,9 @@ force_value_to_type (tree type, tree value)
      Still if we end up with truly mismatched types here, fall back
      to using a VIEW_CONVERT_EXPR or a literal zero to not leak invalid
      GIMPLE to the following passes.  */
+  if (TREE_CODE (value) == WITH_SIZE_EXPR)
+    value = TREE_OPERAND (value, 0);
+  
   if (!is_gimple_reg_type (TREE_TYPE (value))
           || TYPE_SIZE (type) == TYPE_SIZE (TREE_TYPE (value)))
     return fold_build1 (VIEW_CONVERT_EXPR, type, value);
@@ -4017,6 +4020,10 @@ inline_forbidden_p (tree fndecl)
   wi.info = (void *) fndecl;
   wi.pset = &visited_nodes;

+  for (tree parm = DECL_ARGUMENTS (fndecl); parm; parm = DECL_CHAIN (parm))
+    if (TREE_CODE (DECL_SIZE (parm)) != INTEGER_CST)
+      return true;
+  
   FOR_EACH_BB_FN (bb, fun)
     { 
       gimple *ret;

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:7ee164dcfe390fc030028db9112d44255637b1aa

commit r11-7278-g7ee164dcfe390fc030028db9112d44255637b1aa
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Feb 18 12:28:26 2021 +0100

    middle-end/99122 - Issues with VLA parameter inlining

    The following instructs IPA not to inline calls with VLA parameters
    and adjusts inlining not to create invalid view-converted VLA
    parameters on mismatch and makes the error_mark paths with debug
    stmts actually work.

    The first part avoids the ICEs with the testcases already.

    2021-02-18  Richard Biener  <rguenther@suse.de>

            PR middle-end/99122
            * ipa-fnsummary.c (analyze_function_body): Set
            CIF_FUNCTION_NOT_INLINABLE for VLA parameter calls.
            * tree-inline.c (insert_init_debug_bind): Pass NULL for
            error_mark_node values.
            (force_value_to_type): Do not build V_C_Es for WITH_SIZE_EXPR
            values.
            (setup_one_parameter): Delay force_value_to_type until when
            it's needed.

            * gcc.dg/pr99122-1.c: New testcase.
            * gcc.dg/pr99122-2.c: Likewise.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #16 from Martin Jambor <jamborm at gcc dot gnu.org> ---
For the IPA-CP ICE, I am still running some tests, but I am currently leaning
towards the following.  It might in theory disable IPA-CP in some strange K&R
corner cases (I am searching for those with the tests), but I cannot say I care
too much even if it does:

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 167913cb927..4c8b76c09be 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1186,6 +1186,20 @@ set_single_call_flag (cgraph_node *node, void *)
   return false;
 }

+/* Return true if the Ith formal parameter in function described by INFO has a
+   type that is known and safe to accept constants.  */
+
+static bool
+ipa_cp_param_has_safe_type_p (ipa_node_params *info, int i)
+{
+  tree t = ipa_get_type (info, i);
+  if (!t)
+    return false;
+  /* Attempting to propagate to parameters that are VLAs runs afoul
limitations
+     of how clone materialization implementation.  */
+  return TREE_CODE (TYPE_SIZE (t)) == INTEGER_CST;
+}
+
 /* Initialize ipcp_lattices.  */

 static void
@@ -1277,7 +1291,8 @@ initialize_node_lattices (struct cgraph_node *node)
       ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
       if (disable
          || (pre_modified && (surviving_params.length () <= (unsigned) i
-                              || !surviving_params[i])))
+                              || !surviving_params[i]))
+         || !ipa_cp_param_has_safe_type_p (info, i))
        {
          plats->itself.set_to_bottom ();
          plats->ctxlat.set_to_bottom ();

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #16)
> For the IPA-CP ICE, I am still running some tests, but I am currently
> leaning towards the following.  It might in theory disable IPA-CP in some
> strange K&R corner cases (I am searching for those with the tests), but I
> cannot say I care too much even if it does:
> 
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 167913cb927..4c8b76c09be 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1186,6 +1186,20 @@ set_single_call_flag (cgraph_node *node, void *)
>    return false;
>  }
>  
> +/* Return true if the Ith formal parameter in function described by INFO
> has a
> +   type that is known and safe to accept constants.  */
> +
> +static bool
> +ipa_cp_param_has_safe_type_p (ipa_node_params *info, int i)
> +{
> +  tree t = ipa_get_type (info, i);
> +  if (!t)
> +    return false;
> +  /* Attempting to propagate to parameters that are VLAs runs afoul
> limitations
> +     of how clone materialization implementation.  */
> +  return TREE_CODE (TYPE_SIZE (t)) == INTEGER_CST;

there's variably_modified_type_p (you can pass NULL_TREE for the fndecl)
which is more to the point.  Otherwise it looks reasonable.  Does IPA CP
do things like IPA SRA and split aggregates?  I wonder in which cases
IPA CP would derive "constants" for aggregates, so why are aggregate
parameters even tracked?

> +}
> +
>  /* Initialize ipcp_lattices.  */
>  
>  static void
> @@ -1277,7 +1291,8 @@ initialize_node_lattices (struct cgraph_node *node)
>        ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
>        if (disable
>           || (pre_modified && (surviving_params.length () <= (unsigned) i
> -                              || !surviving_params[i])))
> +                              || !surviving_params[i]))
> +         || !ipa_cp_param_has_safe_type_p (info, i))
>         {
>           plats->itself.set_to_bottom ();
>           plats->ctxlat.set_to_bottom ();

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

--- Comment #18 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 50220
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50220&action=edit
patch

I am testing the following for the inline issue for the last testcase, leaving
the IPA CP one to you.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #19 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #17)
> there's variably_modified_type_p (you can pass NULL_TREE for the fndecl)
> which is more to the point.  Otherwise it looks reasonable.  Does IPA CP
> do things like IPA SRA and split aggregates?

No, it does not split them, the only SRAish thing it does is simple
removal of unused register-type parameters.

> I wonder in which cases IPA CP would derive "constants" for
> aggregates, so why are aggregate parameters even tracked?

I am not sure I understand the question.  The testcase in comment #12
attempts to pass a simple double constant to what is actually a VLA
structure, causing an ICE on undefined behavior input, which is the
main thing I want to avoid.

But I have just realized that if we now insist that we know all types
anyway - I have run the whole C testsuite and did not find any K&R
testcase where IPA-CP would consider a parameter for propagation when
not knowing its type - we can do something better and only propagate
when we know that force_value_to_type would not resort to building a
zero constructor.

This will allow to still propagate ("aggregate") constants within VLAs
like if there was one in b.n in the testcase (and the caller was
somewhat saner).

> I am testing the following for the inline issue for the last testcase,
> leaving the IPA CP one to you.

Sure, thanks.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think you don't want variably_modified_type_p, that returns true even for
pointers to VLAs (and references etc., perhaps many times indirectly).
I think those should be fine, they are used fairly often unlike variable length
structures, and so we'd run into issues before.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenther at suse dot de]

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

--- Comment #21 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 19 Feb 2021, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99122
> 
> --- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> I think you don't want variably_modified_type_p, that returns true even for
> pointers to VLAs (and references etc., perhaps many times indirectly).

Ah, indeed.  But then you want !poly_int_tree_p to not disallow
SVE vector types.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[cvs-commit at gcc dot gnu.org]

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

--- Comment #22 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:1a2a7096e5e20d736c6138179470b21aa5a74864

commit r11-7296-g1a2a7096e5e20d736c6138179470b21aa5a74864
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Feb 19 09:38:52 2021 +0100

    middle-end/99122 - more VLA inlining fixes

    This avoids declaring a function with VLA arguments or return values
    as inlineable.  IPA CP still ICEs, so the testcase has that disabled.

    2021-02-19  Richard Biener  <rguenther@suse.de>

            PR middle-end/99122
            * tree-inline.c (inline_forbidden_p): Do not inline functions
            with VLA arguments or return value.

            * gcc.dg/pr99122-3.c: New testcase.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|rguenth at gcc dot gnu.org         |jamborm at gcc dot gnu.org

--- Comment #23 from Richard Biener <rguenth at gcc dot gnu.org> ---
Leaving the IPA-CP parts to Martin.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #24 from Martin Jambor <jamborm at gcc dot gnu.org> ---
*** Bug 99194 has been marked as a duplicate of this bug. ***

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #25 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I have proposed a patch for the IPA-CP part on the mailing list:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566333.html

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[dcb314 at hotmail dot com]

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

David Binderman <dcb314 at hotmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dcb314 at hotmail dot com

--- Comment #26 from David Binderman <dcb314 at hotmail dot com> ---
Quite interestingly, this ice can also be made with C testsuite file
./gcc.dg/pr99122-3.c at -O2

It seems to have been iceing since before 20210214.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[cvs-commit at gcc dot gnu.org]

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

--- Comment #27 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Jambor <jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:f225c6b0c50dc472e0b73b440b572a3bf1514020

commit r11-7817-gf225c6b0c50dc472e0b73b440b572a3bf1514020
Author: Martin Jambor <mjambor@suse.cz>
Date:   Wed Mar 24 20:27:27 2021 +0100

    ipa: Check that scalar types that IPA-CP comes up with are sane (PR99122)

    This patch fixes the last bit of PR 99122 where various bits of IPA
    infrastructure are presented with a program with type mismatches that
    make it have undefined behavior, and when inlining or performing
    IPA-CP, and encountering such mismatch, we basically try to
    VIEW_CONVERT_EXPR whatever the caller has into whatever the callee has
    or simply use an empty constructor if that cannot be done.  This
    however does not work when the callee has VLA parameters because we
    ICE in the process.

    Richi has already disabled inlining for such cases, this patch avoids
    the issue in IPA-CP.  It adds checks that whatever constant the
    propagation arrived at is actually compatible or fold_convertible to
    the callees formal parameer type.  Unlike in the past, we now have
    types of all parameters of functions that we have analyzed, even with
    LTO, and so can do it.

    This should prevent only bogus propagations.  I have looked at the
    effect of the patch on WPA of Firefox and did not have any.

    I have bootstrapped and LTO bootstrapped and tested the patch on
    x86_64-linux.  OK for trunk?  And perhaps later for GCC 10 too?

    Thanks

    gcc/ChangeLog:

    2021-02-26  Martin Jambor  <mjambor@suse.cz>

            PR ipa/99122
            * ipa-cp.c (initialize_node_lattices): Mark as bottom all
            parameters with unknown type.
            (ipacp_value_safe_for_type): New function.
            (propagate_vals_across_arith_jfunc): Verify that the constant type
            can be used for a type of the formal parameter.
            (propagate_vals_across_ancestor): Likewise.
            (propagate_scalar_across_jump_function): Likewise.  Pass the type
            also to propagate_vals_across_ancestor.

    gcc/testsuite/ChangeLog:

    2021-02-26  Martin Jambor  <mjambor@suse.cz>

            PR ipa/99122
            * gcc.dg/pr99122-3.c: Remove -fno-ipa-cp from options.

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

--- Comment #28 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So fixed for GCC 11 now?

[Bug ipa/99122] [10/11 Regression] ICE in force_constant_size, at gimplify.c:733

[jamborm at gcc dot gnu.org]

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

--- Comment #29 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #28)
> So fixed for GCC 11 now?

Yes, it should be fixed in GCC 11.

We talked about backporting the patches to GCC 10 with Richi on IRC today and
decided to wait for potential fallout even if we miss the 10.3 release.

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.3                        |10.4

--- Comment #30 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.3 is being released, retargeting bugs to GCC 10.4.

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #31 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> We talked about backporting the patches to GCC 10 with Richi on IRC today
> and decided to wait for potential fallout even if we miss the 10.3 release.

The fallout is major in Ada for the return part because the logic completely
overlooks the CALL_EXPR_RETURN_SLOT_OPT flag; when it is set, you don't need to
create a variable in the caller when inlining the function, so this works:

procedure Inline22 (L, U : Integer) is

  type Arr is array (Integer range L .. U) of Boolean;

  function Get_Zero return Arr;
  pragma Inline_Always (Get_Zero);

  function Get_Zero return Arr is
  begin
    return (others => False);
  end;

  A : Arr;

begin
  A := Get_Zero;
end;

eric@fomalhaut:~/install/gcc-10/bin/gcc -c inline22.adb 
eric@fomalhaut:~/install/gcc-11/bin/gcc -c inline22.adb 
inline22.adb: In function 'Inline22.Get_Zero':
inline22.adb:10:3: error: function 'Inline22.Get_Zero' can never be inlined
because it has a VLA return argument

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

--- Comment #32 from Richard Biener <rguenth at gcc dot gnu.org> ---
+  /* We cannot inline a function with a VLA typed argument or result since
+     we have no implementation materializing a variable of such type in
+     the caller.  */
+  if (COMPLETE_TYPE_P (TREE_TYPE (TREE_TYPE (fndecl)))
+      && !poly_int_tree_p (TYPE_SIZE (TREE_TYPE (TREE_TYPE (fndecl)))))
+    return true;

so that would mean adding

       && !DECL_BY_REFERENCE (DECL_RESULT (fndecl))

I suppose or looking at DECL_RESULT in the first place (which is a pointer
in that case).

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[ebotcazou at gcc dot gnu.org]

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

--- Comment #33 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> so that would mean adding
> 
>        && !DECL_BY_REFERENCE (DECL_RESULT (fndecl))
> 
> I suppose or looking at DECL_RESULT in the first place (which is a pointer
> in that case).

Not in the array case though:

(gdb) p debug_tree(fndecl->decl_non_common.result)
 <result_decl 0x7ffff581c000 D.4658
    type <array_type 0x7ffff5818bd0 inline22__arr
        type <boolean_type 0x7ffff6934e70 boolean sizes-gimplified public
visited unsigned QI

(gdb) p fndecl->decl_non_common.result->decl_common.decl_by_reference_flag
$16 = 0

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

--- Comment #34 from Richard Biener <rguenth at gcc dot gnu.org> ---
interestingly I see

  a.9_70 = .builtin_alloca_with_align (iftmp.8_1, 8);
  (*a.9_70) = inline22.get_zero (); [static-chain: &FRAME.20] [return slot
optimization]

so there's no WITH_SIZE_EXPR, but the return value ends up done by reference:

        leaq    -80(%rbp), %rdx
        movq    %rdx, %r10
        movq    %rax, %rdi
        call    inline22__get_zero.0
        movq    %rbx, %rsp
        movq    -8(%rbp), %rbx
        leave

so I wonder if omitting DECL_BY_REFERENCE is simply a bug?  I mean the
ABI of the callee must be aware and it seems CALL_EXPR_RETURN_SLOT_OPT
is set by gimplification.  IIRC CALL_EXPR_RETURN_SLOT_OPT also only means
we _may_ return by reference not that we must.

/* In a CALL_EXPR, means that it's safe to use the target of the call
   expansion as the return slot for a call that returns in memory.  */
#define CALL_EXPR_RETURN_SLOT_OPT(NODE) \
  (CALL_EXPR_CHECK (NODE)->base.private_flag)

But of course if we do not exercise the return slot opt then there definitely
is a WITH_SIZE_EXPR missing.  Unless I'm missing somehting ...

The gimplifier code checks

              else if (TREE_CODE (*to_p) != SSA_NAME
                      && (!is_gimple_variable (*to_p)
                          || needs_to_live_in_memory (*to_p)))
                /* Don't use the original target if it's already addressable;
                   if its address escapes, and the called function uses the
                   NRV optimization, a conforming program could see *to_p
                   change before the called function returns; see c++/19317.
                   When optimizing, the return_slot pass marks more functions
                   as safe after we have escape info.  */
                use_target = false;

so I wonder if you can modify the Ada testcase so that
CALL_EXPR_RETURN_SLOT_OPT is not set?

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[ebotcazou at gcc dot gnu.org]

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

--- Comment #35 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> interestingly I see
> 
>   a.9_70 = .builtin_alloca_with_align (iftmp.8_1, 8);
>   (*a.9_70) = inline22.get_zero (); [static-chain: &FRAME.20] [return slot
> optimization]
> 
> so there's no WITH_SIZE_EXPR, but the return value ends up done by reference:
> 
>         leaq    -80(%rbp), %rdx
>         movq    %rdx, %r10
>         movq    %rax, %rdi
>         call    inline22__get_zero.0
>         movq    %rbx, %rsp
>         movq    -8(%rbp), %rbx
>         leave
> 
> so I wonder if omitting DECL_BY_REFERENCE is simply a bug?  I mean the
> ABI of the callee must be aware and it seems CALL_EXPR_RETURN_SLOT_OPT
> is set by gimplification.  IIRC CALL_EXPR_RETURN_SLOT_OPT also only means
> we _may_ return by reference not that we must.

I don't think it's a bug, in the sense that you can be aggregate_value_p
without being DECL_BY_REFERENCE, as it's the case here; the latter is explicit
in the GIMPLE representation whereas the former is not.

> But of course if we do not exercise the return slot opt then there definitely
> is a WITH_SIZE_EXPR missing.  Unless I'm missing somehting ...

WITH_SIZE_EXPR is for something else (self-referential types).

> so I wonder if you can modify the Ada testcase so that
> CALL_EXPR_RETURN_SLOT_OPT is not set?

No, CALL_EXPR_RETURN_SLOT_OPT is always set, that's the point.

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[cvs-commit at gcc dot gnu.org]

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

--- Comment #36 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:ad085ef5fb4142df2027f81ea03992fdafc6e2f6

commit r12-1176-gad085ef5fb4142df2027f81ea03992fdafc6e2f6
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Thu Jun 3 12:39:39 2021 +0200

    Tame fix for PR ipa/99122

    The return part has a major performance impact in Ada where variable-sized
    types are first-class citizens, but it turns out that it is not exercized
    in the testsuite yet, so back it out for now.

    gcc/
            PR ipa/99122
            * tree-inline.c (inline_forbidden_p): Remove test on return type.
    gcc/testsuite/
            * gnat.dg/inline22.adb: New test.

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[cvs-commit at gcc dot gnu.org]

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

--- Comment #37 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:8b1190d527d01dc74ee53e47b0366d01270c330c

commit r11-8508-g8b1190d527d01dc74ee53e47b0366d01270c330c
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Thu Jun 3 12:39:39 2021 +0200

    Tame fix for PR ipa/99122

    The return part has a major performance impact in Ada where variable-sized
    types are first-class citizens, but it turns out that it is not exercized
    in the testsuite yet, so back it out for now.

    gcc/
            PR ipa/99122
            * tree-inline.c (inline_forbidden_p): Remove test on return type.
    gcc/testsuite/
            * gnat.dg/inline22.adb: New test.

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #38 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug ipa/99122] [10 Regression] ICE in force_constant_size, at gimplify.c:733

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.0
      Known to fail|                            |10.5.0
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #39 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed in GCC 11.