> On Aug 16, 2021, at 2:12 AM, Richard Biener <rguent...@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?
To me, treating “call to .DEFERRED_INIT” as “is_gimple_val” is reasonable
since this call actually is a constant.
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 <rguent...@suse.de> wrote:
>>>>
>>>> On August 11, 2021 6:22:00 PM GMT+02:00, Qing Zhao <qing.z...@oracle.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>> On Aug 11, 2021, at 10:53 AM, Richard Biener <rguent...@suse.de> wrote:
>>>>>>
>>>>>> On August 11, 2021 5:30:40 PM GMT+02:00, Qing Zhao
>>>>>> <qing.z...@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 <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
