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

[Richard Biener <rguenther@suse.de>]

On Tue, 10 Aug 2021, Qing Zhao wrote:

> Hi,
> 
> > On Aug 10, 2021, at 9:16 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Tue, 10 Aug 2021, Qing Zhao wrote:
> > 
> >>>>> 
> >>>>> +static void
> >>>>> +expand_DEFERRED_INIT (internal_fn, gcall *stmt)
> >>>>> +{
> >>>>> +  tree var = gimple_call_lhs (stmt);
> >>>>> +  tree size_of_var = gimple_call_arg (stmt, 0);
> >>>>> +  tree vlaaddr = NULL_TREE;
> >>>>> +  tree var_type = TREE_TYPE (var);
> >>>>> +  bool is_vla = (bool) TREE_INT_CST_LOW (gimple_call_arg (stmt, 2));
> >>>>> +  enum auto_init_type init_type
> >>>>> +    = (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg (stmt, 1));
> >>>>> +
> >>>>> +  gcc_assert (init_type > AUTO_INIT_UNINITIALIZED);
> >>>>> +
> >>>>> +  /* if this variable is a VLA, get its SIZE and ADDR first.  */
> >>>>> +  if (is_vla)
> >>>>> +    {
> >>>>> +      /* The temporary address variable for this vla should have been
> >>>>> +        created during gimplification phase.  Refer to gimplify_vla_decl
> >>>>> +        for details.  */
> >>>>> +      tree var_decl = (TREE_CODE (var) == SSA_NAME) ?
> >>>>> +                      SSA_NAME_VAR (var) : var;
> >>>>> +      gcc_assert (DECL_HAS_VALUE_EXPR_P (var_decl));
> >>>>> +      gcc_assert (TREE_CODE (DECL_VALUE_EXPR (var_decl)) == 
> >>>>> INDIRECT_REF);
> >>>>> +      /* Get the address of this vla variable.  */
> >>>>> +      vlaaddr = TREE_OPERAND (DECL_VALUE_EXPR (var_decl), 0);
> >>>>> 
> >>>>> err - isn't the address of the decl represented by the LHS 
> >>>>> regardless whether this is a VLA or not?
> >>>> 
> >>>> The LHS of the call to .DEFERRED_INIT is the DECL itself whatever it’s a VLA or not. 
> >>>> 
> >>>> In order to create a memset call, we need the Address of this DECL as the first argument. 
> >>>> If the DECL is not a VLA, we just simply apply “build_fold_addr_expr” on this DECL to get its address,
> >>>> However, for VLA, during gimplification phase “gimplify_vla_decl”, we have already created a temporary
> >>>> address variable for this DECL, and recorded this address variable with “DECL_VALUE_EXPR(DECL), 
> >>>> We should use this already created address variable  for VLAs. 
> >>> 
> >>> So the issue is that the LHS of the .DEFERRED_INIT call is not properly
> >>> gimplified.  We should not have such decl there but I see we do not
> >>> have IL verification that covers this.
> >> 
> >> Don’t quite understand here:  do you mean all the LHS of .DEFERRED_INIT call are not properly gimplified, or
> >> Only the LHS of .DEFERRED_INIT call for VLA are not properly gimplified?
> > 
> > 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.

> > 
> >> 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.