Martin Jambor <mjam...@suse.cz> writes:
> On Thu, Jul 08 2021, Qing Zhao wrote:
>> (Resend this email since the previous one didn’t quote, I changed one
>> setting in my mail client, hopefully that can fix this issue).
>>
>> Hi, Martin,
>>
>> Thank you for the review and comment.
>>
>>> On Jul 8, 2021, at 8:29 AM, Martin Jambor <mjam...@suse.cz> wrote:
>>>> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>>>> index c05d22f3e8f1..35051d7c6b96 100644
>>>> --- a/gcc/tree-sra.c
>>>> +++ b/gcc/tree-sra.c
>>>> @@ -384,6 +384,13 @@ static struct
>>>>
>>>> /* Numbber of components created when splitting aggregate parameters. */
>>>> int param_reductions_created;
>>>> +
>>>> + /* Number of deferred_init calls that are modified. */
>>>> + int deferred_init;
>>>> +
>>>> + /* Number of deferred_init calls that are created by
>>>> + generate_subtree_deferred_init. */
>>>> + int subtree_deferred_init;
>>>> } sra_stats;
>>>>
>>>> static void
>>>> @@ -4096,6 +4103,110 @@ get_repl_default_def_ssa_name (struct access
>>>> *racc, tree reg_type)
>>>> return get_or_create_ssa_default_def (cfun, racc->replacement_decl);
>>>> }
>>>>
>>>> +
>>>> +/* Generate statements to call .DEFERRED_INIT to initialize scalar
>>>> replacements
>>>> + of accesses within a subtree ACCESS; all its children, siblings and
>>>> their
>>>> + children are to be processed.
>>>> + GSI is a statement iterator used to place the new statements. */
>>>> +static void
>>>> +generate_subtree_deferred_init (struct access *access,
>>>> + tree init_type,
>>>> + tree is_vla,
>>>> + gimple_stmt_iterator *gsi,
>>>> + location_t loc)
>>>> +{
>>>> + do
>>>> + {
>>>> + if (access->grp_to_be_replaced)
>>>> + {
>>>> + tree repl = get_access_replacement (access);
>>>> + gimple *call
>>>> + = gimple_build_call_internal (IFN_DEFERRED_INIT, 3,
>>>> + TYPE_SIZE_UNIT (TREE_TYPE (repl)),
>>>> + init_type, is_vla);
>>>> + gimple_call_set_lhs (call, repl);
>>>> + gsi_insert_before (gsi, call, GSI_SAME_STMT);
>>>> + update_stmt (call);
>>>> + gimple_set_location (call, loc);
>>>> + sra_stats.subtree_deferred_init++;
>>>> + }
>>>> + else if (access->grp_to_be_debug_replaced)
>>>> + {
>>>> + tree drepl = get_access_replacement (access);
>>>> + tree call = build_call_expr_internal_loc
>>>> + (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
>>>> + TREE_TYPE (drepl), 3,
>>>> + TYPE_SIZE_UNIT (TREE_TYPE (drepl)),
>>>> + init_type, is_vla);
>>>> + gdebug *ds = gimple_build_debug_bind (drepl, call,
>>>> + gsi_stmt (*gsi));
>>>> + gsi_insert_before (gsi, ds, GSI_SAME_STMT);
>>>
>>> Is handling of grp_to_be_debug_replaced accesses necessary here? If so,
>>> why? grp_to_be_debug_replaced accesses are there only to facilitate
>>> debug information about a part of an aggregate decl is that is likely
>>> going to be entirely removed - so that debuggers can sometimes show to
>>> users information about what they would contain had they not removed.
>>> It seems strange you need to mark them as uninitialized because they
>>> should not have any consumers. (But perhaps it is also harmless.)
>>
>> This part has been discussed during the 2nd version of the patch, but
>> I think that more discussion might be necessary.
>>
>> In the previous discussion, Richard Sandiford mentioned:
>> (https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568620.html):
>>
>> =====
>>
>> I guess the thing we need to decide here is whether -ftrivial-auto-var-init
>> should affect debug-only constructs too. If it doesn't, exmaining removed
>> components in a debugger might show uninitialised values in cases where
>> the user was expecting initialised ones. There would be no security
>> concern, but it might be surprising.
>>
>> I think in principle the DRHS can contain a call to DEFERRED_INIT.
>> Doing that would probably require further handling elsewhere though.
>>
>> =====
>>
>> I am still not very confident now for this part of the change.
>
> I see. I still tend to think that with or without the generation of
> gimple_build_debug_binds, the debugger would still not display any value
> for the component in question. Without it there would be no information
> about the component at a any place in code affected by this, with it the
> component would be explicitely uninitialized. But OK.
FTR, I don't have a strong opinion here. You know the code better
than I do, so if you think not generating debug binds is better then
let's do that.
Thanks,
Richard
