> On Jun 30, 2021, at 9:39 AM, Richard Biener <rguent...@suse.de> wrote:
>
> On Wed, 30 Jun 2021, Qing Zhao wrote:
>
>>
>>
>> On Jun 30, 2021, at 2:46 AM, Richard Biener
>> <rguent...@suse.de<mailto:rguent...@suse.de>> wrote:
>>
>> On Wed, 30 Jun 2021, Qing Zhao wrote:
>>
>> Hi,
>>
>> I am testing the 4th patch of -ftrivial-auto-var-init with CPU2017 today,
>> and found the following issues:
>>
>> ****In the dump file of “*t.i.031t.objsz1”, we have:
>>
>> <bb 3> :
>> __s1_len_217 = .DEFERRED_INIT (__s1_len_176, 2);
>> __s2_len_218 = .DEFERRED_INIT (__s2_len_177, 2);
>>
>> I looks like this .DEFERRED_INIT initializes an already initialized
>> variable.
>>
>> Yes.
>>
>> For cases like the following:
>>
>> int s2_len;
>> s2_len = 4;
>>
>> i.e, the initialization is not at the declaration.
>>
>> We cannot avoid initialization for such cases.
>
> But I'd have expected
>
> s2_len = .DEFERRED_INIT (s2_len, 0);
> s2_len = 4;
>
> from the above - thus the deferred init _before_ the first
> "use" which is the explicit init. How does the other order
> happen to materialize? As said, I believe it shouldn't.
Right, this is strange to me too. … don’t quite understand.
Maybe I need to debug deeper into “ccp” phase to understand this behavior.
>
>> I'd expect to only ever see default definition SSA names
>> as first argument to .DEFERRED_INIT.
>>
>> You mean something like:
>> __s2_len_218 = .DEFERRED_INIT (__s2_len, 2);
>
> No,
>
> __s2_len_218 = .DEFERRED_INIT (__s2_len_217(D), 2);
Okay, I see.
Then please see the following:
******In “GIMPLE” dump:
try
{
C = .DEFERRED_INIT (C, 2);
syshandle = .DEFERRED_INIT (syshandle, 2);
ba = .DEFERRED_INIT (ba, 2);
{
int i;
i = .DEFERRED_INIT (i, 2);
i = 0;
goto <D.18219>;
<D.18218>:
{
size_t __s1_len;
size_t __s2_len;
__s1_len = .DEFERRED_INIT (__s1_len, 2);
__s2_len = .DEFERRED_INIT (__s2_len, 2);
__s2_len = 7;
if (__s2_len <= 3) goto <D.18230>; else goto <D.18231>;
NOTE, in the above, in addition to “__s2_len”, “I” also has an original
initialization already.
******Then in “ssa” dump:
<bb 2> :
C_200 = .DEFERRED_INIT (C_199(D), 2);
syshandle = .DEFERRED_INIT (syshandle, 2);
ba_204 = .DEFERRED_INIT (ba_203(D), 2);
i_206 = .DEFERRED_INIT (i_205(D), 2);
i_207 = 0;
goto <bb 37>; [INV]
<bb 3> :
__s1_len_217 = .DEFERRED_INIT (__s1_len_176, 2);
__s2_len_218 = .DEFERRED_INIT (__s2_len_177, 2);
__s2_len_219 = 7;
if (__s2_len_219 <= 3)
NOTE, in the above, “I” has:
i_206 = .DEFERRED_INIT (i_205(D), 2);
But, for “__s2_len”, it is:
__s2_len_218 = .DEFERRED_INIT (__s2_len_177, 2);
Not sure why such difference?
>
>> ?
>>
>>
>> __s2_len_219 = 7;
>> if (__s2_len_219 <= 3)
>> goto <bb 4>; [INV]
>> else
>> goto <bb 9>; [INV]
>>
>> <bb 4> :
>> _1 = (long unsigned int) i_175;
>>
>>
>> ****However, after “ccp”, in “t.i.032t.ccp1”, we have:
>>
>> <bb 3> :
>> __s1_len_217 = .DEFERRED_INIT (__s1_len_176, 2);
>> __s2_len_218 = .DEFERRED_INIT (7, 2);
>> _36 = (long unsigned int) i_175;
>> _37 = _36 * 8;
>> _38 = argv_220(D) + _37;
>>
>>
>> Looks like that the optimization “ccp” replaced the first argument of the
>> call .DEFERRED_INIT with the constant 7.
>> This should be avoided.
>>
>> (NOTE, this issue existed in the previous patches, however, only exposed
>> with this version since I added more verification
>> code in tree-cfg.c to verify the call to .DEFERRED_INIT).
>>
>> I am wondering what’s the best solution to this problem?
>>
>> I think you have to trace where this "bogus" .DEFERRED_INIT comes from
>> originally. Or alternatively, if this is unavoidable,
>>
>> This is unavoidable, I believe.
>
> I see but don't believe it yet ;)
In order to add initialization for all possible uninitialized auto variables,
we check whether the auto variable is initialized at the
Declaration, we don’t follow the data flow to check the later possible
initialization. Therefore, we might add more initialization than
Necessary. However, most of such additional initialization should be
eliminated easily by later optimizations. Especially after
the call to .DEFERRED_INIT being expanded to real initialization.
>
>> add "constant
>> folding" of .DEFERRED_INIT so that defered init of an initialized
>> object becomes the object itself, thus retain the previous - eventually
>> partial - initialization only.
>>
>> If this additional .DEFERRED_INIT will be kept till RTL expansion phase,
>> then it will become a real initialization:
>>
>> i.e.
>>
>> s2_len = 0; //.DEFERRED_INIT expanded
>> s2_len = 4; // the original initialization
>>
>> Then the first initialization will be eliminated by current RTL optimization
>> easily, right?
>
> Well, in your example above it's effectively elimiated by GIMPLE
> optimization. IIRC you're using the first argument of .DEFERRED_INIT
> for diagnostic purposes only, correct?
In Previous patches, Yes, the first one only for diagnostic purpose, so we can
just delete the first argument and make the call as
temp = .DEFERRED_INIT (init_type);
Then this should resolve this current issue?
But in this patch, in order to merge VLA and NON_VLA, the “size” info of the
VLA variable will be carried by the first argument as following:
temp = .DEFERRED_INIT (SIZE_EXPR (temp), init_type).
So, we cannot delete the first argument anymore.
Can I change the parameter of .DEFERRED_INIT a little bit as following:
Temp = .DEFERRED_INIT ( NULL or SIZE_EXPR, init_type)
The first parameter might be NULL for non-vla, and a SIZE_EXPR for vla?
thanks.
Qing
>
> Richard.
>
>> Qing
>>
>>
>> Richard.
>>
>> Can we add any attribute to the internal function argument to prevent later
>> optimizations that might applied on it?
>> Or just update “ccp” phase to specially handle calls to .DEFERRED_INIT? (Not
>> sure whether there are other phases have the
>> Same issue?)
>>
>> Let me know if you have any suggestion.
>>
>> Thanks a lot for your help.
>>
>> Qing
>>
>> --
>> Richard Biener <rguent...@suse.de<mailto:rguent...@suse.de>>
>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
>>
>>
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
