Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

[Qing Zhao <qing.zhao@oracle.com>]

Hi, Richard,

> On Aug 10, 2021, at 10:22 AM, Richard Biener <rguenther@suse.de> wrote:
>>> 
>>> Especially in the VLA case but likely also in general (though unlikely
>>> since usually the receiver of initializations are simple enough).  I'd
>>> expect the VLA case end up as
>>> 
>>> *ptr_to_decl = .DEFERRED_INIT (...);
>>> 
>>> where *ptr_to_decl is the DECL_VALUE_EXPR of the decl.
>> 
>> So, for the following small testing case:
>> 
>> ====
>> extern void bar (int);
>> 
>> void foo(int n)
>> {
>>  int arr[n];
>>  bar (arr[2]);
>>  return;
>> }
>> =====
>> 
>> If I compile it with -ftrivial-auto-var-init=zero -fdump-tree-gimple -S -o auto-init-11.s -fdump-rtl-expand, the *.gimple dump is:
>> 
>> =====
>> void foo (int n)
>> {
>>  int n.0;
>>  sizetype D.1950;
>>  bitsizetype D.1951;
>>  sizetype D.1952;
>>  bitsizetype D.1953;
>>  sizetype D.1954;
>>  int[0:D.1950] * arr.1;
>>  void * saved_stack.2;
>>  int arr[0:D.1950] [value-expr: *arr.1];
>> 
>>  saved_stack.2 = __builtin_stack_save ();
>>  try
>>    {
>>      n.0 = n;
>>      _1 = (long int) n.0;
>>      _2 = _1 + -1;
>>      _3 = (sizetype) _2;
>>      D.1950 = _3;
>>      _4 = (sizetype) n.0;
>>      _5 = (bitsizetype) _4;
>>      _6 = _5 * 32;
>>      D.1951 = _6;
>>      _7 = (sizetype) n.0;
>>      _8 = _7 * 4;
>>      D.1952 = _8;
>>      _9 = (sizetype) n.0;
>>      _10 = (bitsizetype) _9;
>>      _11 = _10 * 32;
>>      D.1953 = _11;
>>      _12 = (sizetype) n.0;
>>      _13 = _12 * 4;
>>      D.1954 = _13;
>>      arr.1 = __builtin_alloca_with_align (D.1954, 32);
>>      arr = .DEFERRED_INIT (D.1952, 2, 1);
>>      _14 = (*arr.1)[2];
>>      bar (_14);
>>      return;
>>    }
>>  finally
>>    {
>>      __builtin_stack_restore (saved_stack.2);
>>    }
>> }
>> 
>> ====
>> 
>> You think that the above .DEFEERED_INIT is not correct?
>> It should be:
>> 
>> *arr.1 = .DEFERRED_INIT (D.1952. 2, 1);
>> 
>> ?
> 
> Yes.
> 

I updated gimplify.c for VLA and now it emits the call to .DEFERRED_INIT as:

      arr.1 = __builtin_alloca_with_align (D.1954, 32);
      *arr.1 = .DEFERRED_INIT (D.1952, 2, 1);

However, this call triggered the assertion failure in verify_gimple_call of tree-cfg.c because the LHS is not a valid LHS. 
Then I modify tree-cfg.c as:

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 330eb7dd89bf..180d4f1f9e32 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -3375,7 +3375,11 @@ verify_gimple_call (gcall *stmt)
      }
 
   tree lhs = gimple_call_lhs (stmt);
+  /* For .DEFERRED_INIT call, the LHS might be an indirection of
+     a pointer for the VLA variable, which is not a valid LHS of
+     a gimple call, we ignore the asssertion on this.  */ 
   if (lhs
+      && (!gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
       && (!is_gimple_reg (lhs)
          && (!is_gimple_lvalue (lhs)
              || verify_types_in_gimple_reference

The assertion failure in tree-cfg.c got resolved, but I got another assertion failure in operands_scanner::get_expr_operands (tree *expr_p, int flags), line 945:

 939   /* If we get here, something has gone wrong.  */
 940   if (flag_checking)
 941     {
 942       fprintf (stderr, "unhandled expression in get_expr_operands():\n");
 943       debug_tree (expr);
 944       fputs ("\n", stderr);
 945       gcc_unreachable ();
 946     }

Looks like that  the gimple statement:
    *arr.1 = .DEFERRED_INIT (D.1952, 2, 1);

Is not valid.  i.e, the LHS should not be an indirection to a pointer. 

How to resolve this issue?

Thanks a lot for your help.

Qing
>>> 
>>>> What do you mean by “such” decl? A decl whole “DECL_VALUE_EXPR(DECL)” is valid?
>>> 
>>> A 'decl' that has a DECL_VALUE_EXPR should not appear in the IL, it should
>>> always be refered to as its DECL_VALUE_EXPR.
>> 
>> Okay.
> 
> I'm going to test
> 
> diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
> index ebf7eea3b04..15c73b6d6f4 100644
> --- a/gcc/tree-ssa-operands.c
> +++ b/gcc/tree-ssa-operands.c
> @@ -799,10 +799,11 @@ operands_scanner::get_expr_operands (tree *expr_p, 
> int flags)
>                         flags | opf_not_non_addressable | 
> opf_address_taken);
>       return;
> 
> -    case SSA_NAME:
>     case VAR_DECL:
>     case PARM_DECL:
>     case RESULT_DECL:
> +      gcc_checking_assert (!DECL_HAS_VALUE_EXPR_P (expr));
> +    case SSA_NAME:
>     case STRING_CST:
>     case CONST_DECL:
>       if (!(flags & opf_address_taken))
> 
> which should pass on unmodified trunk (fingers crossing ;)), but
> it would likely trip on the current -ftrivial-auto-init patch.
> 
> The issue with the current IL is that nothing keeps arr.1 live
> and thus the allocation could be DCEd but the .DEFERRED_INIT
> call would remain, eventually being expanded to zero storage
> that isn't there.
> 
> Richard.