Hi,
On Mon, Jul 26 2021, Qing Zhao wrote:
> HI, Martin,
>
> Thank you for the comments and suggestions on tree-sra.c changes.
>
>>> ******Compare with the 4th version, the following are the major changes:
>>>
>>> 1. delete the code for handling "grp_to_be_debug_replaced" since they are
>>> not needed per Martin Jambor's suggestion.
>>
>> sorry if I did not make myself clear in my last email, but the deferred
>> init calls should not result into setting any bits in
>> cannot_scalarize_away_bitmap in the SRA pass, otherwise you'll get
>> different optimization with and without -ftrivial-auto-var-init.
>
> It’s my bad that I missed this part of your comments…
>
>>
>> So you either need to change build_access_from_expr like I described in
>> my email
>
> Is the following the change you suggested previously:
>
> [opc@qinzhao-ol8u3-x86 gcc]$ git diff tree-sra.c
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index d1280e5f8848..c2597b705169 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -1195,8 +1195,10 @@ build_access_from_expr (tree expr, gimple *stmt, bool
> write)
> {
> /* 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)
> + replacement for everything. However, when the STMT is a call to
> + DEFERRED_INIT, we should not set this bit. */
> + 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;
> }
>
Yes, although I think that the one I wrote today is more efficient as it
tests for IFN_DEFERRED_INIT only if we already know stmt is a call and
also philosophically more correct as the test is performed only for the
LHS of the statement (I don't think either reason matters much in
practice, though).
>
>> or add the following to your patch, which is probably slightly
>> mor efficient (but it has been only very mildly tested).
>>
>>
>> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>> index d1280e5f884..602b0fb3a2d 100644
>> --- a/gcc/tree-sra.c
>> +++ b/gcc/tree-sra.c
>> @@ -1395,7 +1395,12 @@ scan_function (void)
>>
>> t = gimple_call_lhs (stmt);
>> if (t && !disqualify_if_bad_bb_terminating_stmt (stmt, t, NULL))
>> - ret |= build_access_from_expr (t, stmt, true);
>> + {
>> + if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
>> + ret |= !!build_access_from_expr_1 (t, stmt, true);
>> + else
>> + ret |= build_access_from_expr (t, stmt, true);
>> + }
>> break;
>
> Thanks for the patch, but I don’t quite understand the above change:
>
> When the call is IFN_DEFERRED_INIT, you used build_access_from_expr_1 instead
> of build_access_from_expr to avoid setting “cannot_scalarize_away_bitmap” bit.
> But why adding “!” To this call?
Note that it is a double !! which is basically a fancy way to convert a
test for non-NULL to a bool. It is equivalent to:
+ if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
+ ret |= (build_access_from_expr_1 (t, stmt, true) != NULL);
+ else
+ ret |= build_access_from_expr (t, stmt, true);
use whichever variant you like better. But the !! trick is used
throughout the gcc source already.
Martin
