(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.
My questions:
1. If we don’t handle grp_to_be_debug_replaced at all, what will happen? ( the
user of the debugger will see uninitialized values in
the removed part of the aggregate? Or something else?)
2. On the other hand, if we handle grp_to_be_debug_replaced as the current
patch, what will the user of the debugger see?
>
> On a related note, if the intent of the feature is for optimizers to
> behave (almost?) as if it was not taking place,
What’s you mean by “it” here?
> I believe you need to
> handle specially, and probably just ignore, calls to IFN_DEFERRED_INIT
> in scan_function in tree-sra.c.
Do you mean to let tree-sra phase ignore IFN_DEFERRED_INIT calls completely?
My major purpose of change tree-sra.c phase is:
Change:
tmp = .DEFERRED_INIT (24, 2, 0)
To
tmp1 = .DEFERRED_INIT (8, 2, 0);
tmp2 = .DEFERRED_INIT (8, 2, 0);
tmp3 = .DEFERRED_INIT (8, 2, 0);
Doing this is to reduce the stack usage.
> Otherwise the generated SRA access
> structures will have extra write flags turned on in them and that will
> lead to different behavior of the pass.
Could you please explain this more?
Thanks.
Qing
>
> Martin
>
>
