> On Aug 16, 2021, at 10:08 AM, Richard Biener <rguent...@suse.de> wrote:
>
> On August 16, 2021 4:48:16 PM GMT+02:00, Qing Zhao <qing.z...@oracle.com>
> wrote:
>>
>>
>>> 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?
>
> 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.
Okay. I see.
>
>> 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.
Thank you!.
Qing
>
> 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 <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)
>>
>
