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

[Richard Biener <rguenther@suse.de>]

On August 16, 2021 4:48:16 PM GMT+02:00, Qing Zhao <qing.zhao@oracle.com> wrote:
>
>
>> On Aug 16, 2021, at 2:12 AM, Richard Biener <rguenther@suse.de> wrote:
>> 
>> On Wed, 11 Aug 2021, Qing Zhao wrote:
>> 
>>> Hi, 
>>> 
>>> I finally decided to take another approach to resolve this issue, it resolved all the potential issues with the “address taken” auto variable.
>>> 
>>> The basic idea is to avoid generating the temporary variable in the beginning. 
>>> As you mentioned, "The reason is that alt_reloc is memory (because it is address taken) and that GIMPLE says that register typed stores 
>>> need to use a is_gimple_val RHS which the call is not.”
>>> In order to avoid generating the temporary variable for “address taken” auto variable, I updated the utility routine “is_gimple_val” as following:
>>> 
>>> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
>>> index a2563a45c37d..d5ef1aef8cea 100644
>>> --- a/gcc/gimple-expr.c
>>> +++ b/gcc/gimple-expr.c
>>> @@ -787,8 +787,20 @@ is_gimple_reg (tree t)
>>>   return !DECL_NOT_GIMPLE_REG_P (t);
>>> }
>>> 
>>> +/* Return true if T is a call to .DEFERRED_INIT internal function.  */ 
>>> +static bool
>>> +is_deferred_init_call (tree t)
>>> +{
>>> +  if (TREE_CODE (t) == CALL_EXPR
>>> +      &&  CALL_EXPR_IFN (t) == IFN_DEFERRED_INIT)
>>> +    return true;
>>> +  return false;
>>> +}
>>> +
>>> 
>>> -/* Return true if T is a GIMPLE rvalue, i.e. an identifier or a constant.  */
>>> +/* Return true if T is a GIMPLE rvalue, i.e. an identifier or a constant,
>>> +   or a call to .DEFERRED_INIT internal function because the call to
>>> +   .DEFERRED_INIT will eventually be expanded as a constant.  */
>>> 
>>> bool
>>> is_gimple_val (tree t)
>>> @@ -799,7 +811,8 @@ is_gimple_val (tree t)
>>>       && !is_gimple_reg (t))
>>>     return false;
>>> 
>>> -  return (is_gimple_variable (t) || is_gimple_min_invariant (t));
>>> +  return (is_gimple_variable (t) || is_gimple_min_invariant (t)
>>> +         || is_deferred_init_call (t));
>>> }
>>> 
>>> With this change, the temporary variable will not be created for “address taken” auto variable, and uninitialized analysis does not need any change. 
>>> Everything works well.
>>> 
>>> And I believe that treating “call to .DEFERRED_INIT” as “is_gimple_val” is reasonable since this call actually is a constant.
>>> 
>>> Let me know if you have any objection on this solution.
>> 
>> Yeah, I object to this solution.
>
>Can you explain what’s the major issue for this solution? 

It punches a hole into the GIMPLE IL which is very likely incomplete and will cause issues elsewhere. In particular the corresponding type check is missing and not computable since the RHS of this call isn't separately available. 

If you go down this route then you should have added a new constant class tree code instead of an internal function call. 

>To me,  treating “call to .DEFERRED_INIT” as “is_gimple_val” is reasonable since this call actually is a constant.

Sure, but it's not represented as such. 

Richard. 

>Thanks.
>
>Qing
>> Richard.
>> 
>>> thanks.
>>> 
>>> Qing
>>> 
>>>> On Aug 11, 2021, at 3:30 PM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>> 
>>>> Hi, 
>>>> 
>>>> I met another issue for “address taken” auto variable, see below for details:
>>>> 
>>>> **** the testing case: (gcc/testsuite/gcc.dg/uninit-16.c)
>>>> 
>>>> int foo, bar;
>>>> 
>>>> static
>>>> void decode_reloc(int reloc, int *is_alt)
>>>> {
>>>> if (reloc >= 20)
>>>>     *is_alt = 1;
>>>> else if (reloc >= 10)
>>>>     *is_alt = 0;
>>>> }
>>>> 
>>>> void testfunc()
>>>> {
>>>> int alt_reloc;
>>>> 
>>>> decode_reloc(foo, &alt_reloc);
>>>> 
>>>> if (alt_reloc) /* { dg-warning "may be used uninitialized" } */
>>>>   bar = 42;
>>>> }
>>>> 
>>>> ****When compiled with -ftrivial-auto-var-init=zero -O2 -Wuninitialized -fdump-tree-all:
>>>> 
>>>> .*************gimple dump:
>>>> 
>>>> void testfunc ()
>>>> { 
>>>> int alt_reloc;
>>>> 
>>>> try
>>>>   {
>>>>     _1 = .DEFERRED_INIT (4, 2, 0);
>>>>     alt_reloc = _1;
>>>>     foo.0_2 = foo;
>>>>     decode_reloc (foo.0_2, &alt_reloc);
>>>>     alt_reloc.1_3 = alt_reloc;
>>>>     if (alt_reloc.1_3 != 0) goto <D.1952>; else goto <D.1953>;
>>>>     <D.1952>:
>>>>     bar = 42;
>>>>     <D.1953>:
>>>>   }
>>>> finally
>>>>   {
>>>>     alt_reloc = {CLOBBER};
>>>>   }
>>>> }
>>>> 
>>>> **************fre1 dump:
>>>> 
>>>> void testfunc ()
>>>> {
>>>> int alt_reloc;
>>>> int _1;
>>>> int foo.0_2;
>>>> 
>>>> <bb 2> :
>>>> _1 = .DEFERRED_INIT (4, 2, 0);
>>>> foo.0_2 = foo;
>>>> if (foo.0_2 > 19)
>>>>   goto <bb 3>; [50.00%]
>>>> else
>>>>   goto <bb 4>; [50.00%]
>>>> 
>>>> <bb 3> :
>>>> goto <bb 7>; [100.00%]
>>>> 
>>>> <bb 4> :
>>>> if (foo.0_2 > 9)
>>>>   goto <bb 5>; [50.00%]
>>>> else
>>>>   goto <bb 6>; [50.00%]
>>>> 
>>>> <bb 5> :
>>>> goto <bb 8>; [100.00%]
>>>> 
>>>> <bb 6> :
>>>> if (_1 != 0)
>>>>   goto <bb 7>; [INV]
>>>> else
>>>>   goto <bb 8>; [INV]
>>>> 
>>>> <bb 7> :
>>>> bar = 42;
>>>> 
>>>> <bb 8> :
>>>> return;
>>>> 
>>>> }
>>>> 
>>>> From the above IR file after “FRE”, we can see that the major issue with this IR is:
>>>> 
>>>> The address taken auto variable “alt_reloc” has been completely replaced by the temporary variable “_1” in all
>>>> the uses of the original “alt_reloc”. 
>>>> 
>>>> The major problem with such IR is,  during uninitialized analysis phase, the original use of “alt_reloc” disappeared completely.
>>>> So, the warning cannot be reported.
>>>> 
>>>> 
>>>> My questions:
>>>> 
>>>> 1. Is it possible to get the original “alt_reloc” through the temporary variable “_1” with some available information recorded in the IR?
>>>> 2. If not, then we have to record the relationship between “alt_reloc” and “_1” when the original “alt_reloc” is replaced by “_1” and get such relationship during
>>>>   Uninitialized analysis phase.  Is this doable?
>>>> 3. Looks like that for “address taken” auto variable, if we have to introduce a new temporary variable and split the call to .DEFERRED_INIT into two:
>>>> 
>>>>     temp = .DEFERRED_INIT (4, 2, 0);
>>>>     alt_reloc = temp;
>>>> 
>>>>  More issues might possible.
>>>> 
>>>> Any comments and suggestions on this issue?
>>>> 
>>>> Qing
>>>> 
>>>> j
>>>>> On Aug 11, 2021, at 11:55 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>> 
>>>>> On August 11, 2021 6:22:00 PM GMT+02:00, Qing Zhao <qing.zhao@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Aug 11, 2021, at 10:53 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>>>> 
>>>>>>> On August 11, 2021 5:30:40 PM GMT+02:00, Qing Zhao <qing.zhao@oracle.com> wrote:
>>>>>>>> I modified the routine “gimple_add_init_for_auto_var” as the following:
>>>>>>>> ====
>>>>>>>> /* Generate initialization to automatic variable DECL based on INIT_TYPE.
>>>>>>>> Build a call to internal const function DEFERRED_INIT:
>>>>>>>> 1st argument: SIZE of the DECL;
>>>>>>>> 2nd argument: INIT_TYPE;
>>>>>>>> 3rd argument: IS_VLA, 0 NO, 1 YES;
>>>>>>>> 
>>>>>>>> as DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA).  */
>>>>>>>> static void
>>>>>>>> gimple_add_init_for_auto_var (tree decl,
>>>>>>>>                          enum auto_init_type init_type,
>>>>>>>>                          bool is_vla,
>>>>>>>>                          gimple_seq *seq_p)
>>>>>>>> {
>>>>>>>> gcc_assert (VAR_P (decl) && !DECL_EXTERNAL (decl) && !TREE_STATIC (decl));
>>>>>>>> gcc_assert (init_type > AUTO_INIT_UNINITIALIZED);
>>>>>>>> tree decl_size = TYPE_SIZE_UNIT (TREE_TYPE (decl));
>>>>>>>> 
>>>>>>>> tree init_type_node
>>>>>>>> = build_int_cst (integer_type_node, (int) init_type);
>>>>>>>> tree is_vla_node
>>>>>>>> = build_int_cst (integer_type_node, (int) is_vla);
>>>>>>>> 
>>>>>>>> tree call = build_call_expr_internal_loc (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
>>>>>>>>                                        TREE_TYPE (decl), 3,
>>>>>>>>                                        decl_size, init_type_node,
>>>>>>>>                                        is_vla_node);
>>>>>>>> 
>>>>>>>> /* If this DECL is a VLA, a temporary address variable for it has been
>>>>>>>> created, the replacement for DECL is recorded in DECL_VALUE_EXPR (decl),
>>>>>>>> we should use it as the LHS of the call.  */
>>>>>>>> 
>>>>>>>> tree lhs_call
>>>>>>>> = is_vla ? DECL_VALUE_EXPR (decl) : decl;
>>>>>>>> gimplify_assign (lhs_call, call, seq_p);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> With this change, the current issue is resolved, the gimple dump now is:
>>>>>>>> 
>>>>>>>> (*arr.1) = .DEFERRED_INIT (D.1952, 2, 1);
>>>>>>>> 
>>>>>>>> However, there is another new issue:
>>>>>>>> 
>>>>>>>> For the following testing case:
>>>>>>>> 
>>>>>>>> ======
>>>>>>>> [opc@qinzhao-ol8u3-x86 gcc]$ cat t.c
>>>>>>>> int bar;
>>>>>>>> 
>>>>>>>> extern void decode_reloc(int *);
>>>>>>>> 
>>>>>>>> void testfunc()
>>>>>>>> {
>>>>>>>> int alt_reloc;
>>>>>>>> 
>>>>>>>> decode_reloc(&alt_reloc);
>>>>>>>> 
>>>>>>>> if (alt_reloc) /* { dg-warning "may be used uninitialized" } */
>>>>>>>> bar = 42; 
>>>>>>>> }
>>>>>>>> =====
>>>>>>>> 
>>>>>>>> In the above, the auto var “alt_reloc” is address taken, then the gimple dump for it when compiled with -ftrivial-auto-var-init=zero is:
>>>>>>>> 
>>>>>>>> void testfunc ()
>>>>>>>> {
>>>>>>>> int alt_reloc;
>>>>>>>> 
>>>>>>>> try
>>>>>>>> {
>>>>>>>>  _1 = .DEFERRED_INIT (4, 2, 0);
>>>>>>>>  alt_reloc = _1;
>>>>>>>>  decode_reloc (&alt_reloc);
>>>>>>>>  alt_reloc.0_2 = alt_reloc;
>>>>>>>>  if (alt_reloc.0_2 != 0) goto <D.1949>; else goto <D.1950>;
>>>>>>>>  <D.1949>:
>>>>>>>>  bar = 42;
>>>>>>>>  <D.1950>:
>>>>>>>> }
>>>>>>>> finally
>>>>>>>> {
>>>>>>>>  alt_reloc = {CLOBBER};
>>>>>>>> }
>>>>>>>> }
>>>>>>>> 
>>>>>>>> I.e, instead of the expected IR:
>>>>>>>> 
>>>>>>>> alt_reloc = .DEFERRED_INIT (4, 2, 0);
>>>>>>>> 
>>>>>>>> We got the following:
>>>>>>>> 
>>>>>>>> _1 = .DEFERRED_INIT (4, 2, 0);
>>>>>>>>  alt_reloc = _1;
>>>>>>>> 
>>>>>>>> I guess the temp “_1” is created because “alt_reloc” is address taken. 
>>>>>>> 
>>>>>>> Yes and no. The reason is that alt_reloc is memory (because it is address taken) and that GIMPLE says that register typed stores need to use a is_gimple_val RHS which the call is not.
>>>>>> 
>>>>>> Okay.
>>>>>>> 
>>>>>>>> My questions:
>>>>>>>> 
>>>>>>>> Shall we accept such IR for .DEFERRED_INIT purpose when the auto var is address taken? 
>>>>>>> 
>>>>>>> I think so. Note it doesn't necessarily need address taking but any other reason that prevents SSA rewriting the variable suffices. 
>>>>>> 
>>>>>> You mean, in addition to “address taken”, there are other situations that will introduce such IR:
>>>>>> 
>>>>>> temp = .DEFERRED_INIT();
>>>>>> auto_var = temp;
>>>>>> 
>>>>>> So, such IR is unavoidable and we have to handle it?
>>>>> 
>>>>> Yes. 
>>>>> 
>>>>>> If we have to handle it,  what’ the best way to do it?
>>>>>> 
>>>>>> The solution in my mind is:
>>>>>> 1. During uninitialized analysis phase, following the data flow to connect .DEFERRED_INIT to “auto_var”, and then decide that “auto_var” is uninitialized.
>>>>> 
>>>>> Yes. Basically if there's an artificial variable auto initialized you have to look at its uses. 
>>>>> 
>>>>>> 2. During RTL expansion, following the data flow to connect .DEFERRED_INIT to “auto_var”, and then delete “temp”, and then expand .DEFERRED_INIT to auto_var.
>>>>> 
>>>>> That shouldn't be necessary. You'd initialize a temporary register which is then copied to the real variable. That's good enough and should be optimized by the RTL pipeline. 
>>>>> 
>>>>>> Let me know your comments and suggestions on this.
>>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> The only other option is to force. DEFERED_INIT making the LHS address taken which I think could be achieved by passing it the address as argument instead of having a LHS. But let's not go down this route - it will have quite bad behavior on alias analysis and optimization. 
>>>>>> 
>>>>>> Okay.
>>>>>> 
>>>>>> Qing
>>>>>>> 
>>>>>>>> If so, “uninitialized analysis” phase need to be further adjusted to specially handle such IR. 
>>>>>>>> 
>>>>>>>> If not, what should we do when the auto var is address taken?
>>> 
>>> 
>> 
>> -- 
>> Richard Biener <rguenther@suse.de>
>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
>