Hi,
On Mon, Jul 12 2021, Qing Zhao wrote:
>> On Jul 12, 2021, at 2:51 AM, Richard Sandiford <richard.sandif...@arm.com>
>> wrote:
>>
>> 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.
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;
}
