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

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

> On Aug 10, 2021, at 10:22 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> 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.
I will try this. 
> 
> 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.

Okay, I see now.
Will updated the VLA part in gimplify.c and internal-fn.c.

Qing

> 
> Richard.