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

[Richard Sandiford <richard.sandiford@arm.com>]

Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> On Aug 10, 2021, at 3:16 PM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> 
>> 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?
>
> I came up with the following solution:
>
> Define the IFN_DEFERRED_INIT function as:
>
>    LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA);
>
>    if IS_VLA is false, the LHS is the DECL itself,
>    if IS_VLA is true, the LHS is the pointer to this DECL that created by
>    gimplify_vla_decl.
>
>
> The benefit of this solution are:
>
> 1. Resolved the invalid IR issue;
> 2. The call stmt carries the address of the VLA natually;
>
> The issue with this solution is:
>
> For VLA and non-VLA, the LHS will be different, 
>
> Do you see any other potential issues with this solution?

The idea behind the DECL version of the .DEFERRED_INIT semantics was
that .DEFERRED_INIT just returns a SIZE-byte value that the caller
then assigns to a SIZE-byte lhs (with the caller choosing the lhs).
.DEFEREED_INIT itself doesn't read or write memory and so can be const,
which in turn allows alias analysis to be more precise.

If we want to handle the VLA case using pointers instead then I think
that needs to be a different IFN.

If we did handle the VLA case using pointers (not expressing an opinion
on that), then it would be the caller's job to allocate the VLA and work
out the address of the VLA; this isn't something that .DEFERRED_INIT
would work out on the caller's behalf.  The address of the VLA should
therefore be an argument to the new IFN, rather than something that
the IFN returns.

Thanks,
Richard