[patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Qing Zhao
qing.zhao@oracle.com
Mon Jul 12 18:13:57 GMT 2021
Hi, Martin,
Thanks a lot for your experiments and examples, they are really helpful.
So, based on your study, I will delete the code that handle
grp_to_be_debug_replaced accesses in generate_subtree_deferred_init.
Let me know if you have further comments on this.
Qing
> On Jul 12, 2021, at 12:06 PM, Martin Jambor <mjambor@suse.cz> wrote:
>
> Hi,
>
> On Mon, Jul 12 2021, Qing Zhao wrote:
>>> On Jul 12, 2021, at 2:51 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
>>>
>>> Martin Jambor <mjambor@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 <mjambor@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.
>
> I am very flattered that you think I understand debug_binds well :-) (I don't)
>
>>
>> I am okay with not generating debug binds here.
>>
>> Then I will just delete the part of code that guarded with if (access->grp_to_be_debug_replaced)?
>>
>
> But I have done some simple experiments and reached the conclusion the
> code is never executed and wrong. It can get executed if you change the
> if statement in build_access_from_expr like I explained in my previous
> email:
>
> static bool
> build_access_from_expr (tree expr, gimple *stmt, bool write)
> {
> struct access *access;
>
> access = build_access_from_expr_1 (expr, stmt, write);
> if (access)
> {
> /* This means the aggregate is accesses as a whole in a way other than an
> assign statement and thus cannot be removed even if we had a scalar
> replacement for everything. */
> if (cannot_scalarize_away_bitmap
> && !gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
> bitmap_set_bit (cannot_scalarize_away_bitmap, DECL_UID (access->base));
> return true;
> }
> return false;
> }
>
> But then it ICEs when the operand scanner attempts to grok the debug bind:
>
> test.c:43:1: internal compiler error: in get_expr_operands, at tree-ssa-operands.c:945
> 43 | }
>
> Gdb reveals the statement is what I suspected:
>
> #2 0x000000000156eea6 in operands_scanner::parse_ssa_operands (this=0x7fffffffdad0)
> at /home/mjambor/gcc/mine/src/gcc/tree-ssa-operands.c:973
> 973 get_expr_operands (gimple_debug_bind_get_value_ptr (stmt),
> (gdb) pgg stmt
> # DEBUG s2D.1971 => .DEFERRED_INIT (4, 1, 0)
>
> It turns out debug binds cannot have CALL_EXPRs as operands.
>
> So yes, just do not attempt to handle grp_to_be_debug_replaced accesses
> in generate_subtree_deferred_init. In fact, if you do that (+ the
> modification described above) the results seem to be rather good on the
> attached testcase (which I derived from gcc.dg/guality/pr59776.c), at
> least on GIMPLE and at least when compiled with -O1 -g. I did not
> actually attempt to look at the generated dwarf. I have made a simple
> attempt to print uninitialized (and unused) SRAed structure in gdb with
> and without -ftrivial-auto-var-init=pattern, but in both cases it just
> said it was optimized out.
>
> Martin
>
> #include "nop.h"
>
> struct S { float f, g; };
>
> __attribute__((noipa)) void
> foo (struct S *p, int flag)
> {
> struct S s1, s2;
> if (flag)
> {
> s1 = *p;
> s2 = s1;
> }
> else
> {
> s2 = s1;
> }
>
> *(int *) &s2.f = 0;
> asm volatile (NOP : : : "memory");
> asm volatile (NOP : : : "memory");
> s2 = s1;
> asm volatile (NOP : : : "memory");
> asm volatile (NOP : : : "memory");
> }
>
> int __attribute__((noipa))
> getint(void)
> {
> return 1;
> }
>
> int
> main ()
> {
> struct S x = { 5.0f, 6.0f };
> foo (&x, getint());
> return 0;
> }
More information about the Gcc-patches
mailing list