> On Jun 30, 2021, at 12:36 PM, Richard Biener <rguent...@suse.de> wrote:
>
> On June 30, 2021 7:20:18 PM GMT+02:00, Andrew Pinski <pins...@gmail.com>
> wrote:
>> On Wed, Jun 30, 2021 at 8:47 AM Qing Zhao via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> I came up with a very simple testing case that can repeat the same
>> issue:
>>>
>>> [qinzhao@localhost gcc]$ cat t.c
>>> extern void bar (int);
>>> void foo (int a)
>>> {
>>> int i;
>>> for (i = 0; i < a; i++) {
>>> if (__extension__({int size2;
>>> size2 = 4;
>>> size2 > 5;}))
>>> bar (a);
>>> }
>>> }
>>
>> You should show the full dump,
>> What we have is the following:
>>
>>
>>
>> size2_3 = PHI <size2_1(D), size2_13>
>> <bb 3> :
>>
>> size2_12 = .DEFERRED_INIT (size2_3, 2);
>> size2_13 = 4;
>>
>> So CCP decides to propagate 4 into the PHI and then decides size2_1(D)
>> is undefined so size2_3 is then considered 4 and propagates it into
>> the .DEFERRED_INIT.
>
> Which means the DEFERED_INIT is inserted at the wrong place.
Then, where is the correct place for “.DEFERRED_INIT(size2,2)?
The variable “size2” is a block scope variable which is declared inside the
“if” condition:
>>> if (__extension__({int size2;
>>> size2 = 4;
>>> size2 > 5;}))
So, it’s reasonable to insert the initialization inside this block and
immediately after the declaration, This is what the patch currently does:
*****The full dump of “gimple” phase is:
void foo (int a)
{
int D.2240;
int i;
i = .DEFERRED_INIT (i, 2);
i = 0;
goto <D.2244>;
<D.2243>:
{
int size2;
size2 = .DEFERRED_INIT (size2, 2);
size2 = 4;
_1 = size2 > 5;
D.2240 = (int) _1;
}
if (D.2240 != 0) goto <D.2246>; else goto <D.2247>;
<D.2246>:
bar (a);
<D.2247>:
i = i + 1;
<D.2244>:
if (i < a) goto <D.2243>; else goto <D.2241>;
<D.2241>:
}
However, I suspect that the SSA phase moved the “size2” out of its block scope
as following:
******The full “SSA” dump is:
;; Function foo (foo, funcdef_no=0, decl_uid=2236, cgraph_uid=1, symbol_order=0)
void foo (int a)
{
int size2;
int i;
_Bool _1;
int _14;
<bb 2> :
i_7 = .DEFERRED_INIT (i_6(D), 2);
i_8 = 0;
goto <bb 6>; [INV]
<bb 3> :
size2_12 = .DEFERRED_INIT (size2_3, 2);
size2_13 = 4;
_1 = size2_13 > 5;
_14 = (int) _1;
if (_14 != 0)
goto <bb 4>; [INV]
else
goto <bb 5>; [INV]
<bb 4> :
bar (a_11(D));
<bb 5> :
i_16 = i_2 + 1;
<bb 6> :
# i_2 = PHI <i_8(2), i_16(5)>
# size2_3 = PHI <size2_9(D)(2), size2_13(5)>
if (i_2 < a_11(D))
goto <bb 3>; [INV]
else
goto <bb 7>; [INV]
<bb 7> :
return;
}
In the above, we can see that “ # size2_3 = PHI <size2_9(D)(2), size2_13(5)>”
is outside of its block scope already.
“size2” should not be in the same scope as “I" .
This looks incorrect SSA transformation to me.
What do you think?
Qing
>
> Richard.
>>
>> Thanks,
>> Andrew
>>
>>
>>>
>>> [qinzhao@localhost gcc]$
>> /home/qinzhao/Work/GCC/gcc_build_debug/gcc/xgcc
>> -B/home/qinzhao/Work/GCC/gcc_build_debug/gcc/ -std=c99 -m64
>> -march=native -ftrivial-auto-var-init=zero t.c -fdump-tree-all -O1
>>> t.c: In function ‘foo’:
>>> t.c:11:1: error: ‘DEFFERED_INIT’ calls should have the same LHS as
>> the first argument
>>> 11 | }
>>> | ^
>>> size2_12 = .DEFERRED_INIT (4, 2);
>>> during GIMPLE pass: ccp
>>> dump file: a-t.c.032t.ccp1
>>> t.c:11:1: internal compiler error: verify_gimple failed
>>> 0x143ee47 verify_gimple_in_cfg(function*, bool)
>>> ../../latest_gcc/gcc/tree-cfg.c:5501
>>> 0x122d799 execute_function_todo
>>> ../../latest_gcc/gcc/passes.c:2042
>>> 0x122c74b do_per_function
>>> ../../latest_gcc/gcc/passes.c:1687
>>> 0x122d986 execute_todo
>>> ../../latest_gcc/gcc/passes.c:2096
>>> Please submit a full bug report,
>>> with preprocessed source if appropriate.
>>> Please include the complete backtrace with any bug report.
>>> See <https://gcc.gnu.org/bugs/> for instructions.
>>>
>>> In this testing case, both “I” and “size2” are auto vars that are not
>> initialized at declaration but are initialized later by assignment.
>>> However, “I” doesn’t have any issue, but “size2” has such issue.
>>>
>>> ******“ssa” dump:
>>>
>>> <bb 2> :
>>> i_7 = .DEFERRED_INIT (i_6(D), 2);
>>> i_8 = 0;
>>> goto <bb 6>; [INV]
>>>
>>> <bb 3> :
>>> size2_12 = .DEFERRED_INIT (size2_3, 2);
>>> size2_13 = 4;
>>>
>>> ******”ccp1” dump:
>>>
>>> <bb 2> :
>>> i_7 = .DEFERRED_INIT (i_6(D), 2);
>>> goto <bb 4>; [INV]
>>>
>>> <bb 3> :
>>> size2_12 = .DEFERRED_INIT (4, 2);
>>>
>>> So, I am wondering: Is it possible that “ssa” phase have a bug ?
>>>
>>> Qing
>>>
>>>> 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.
>>>>
>>>>> 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);
>>>>
>>>>> ?
>>>>>
>>>>>
>>>>> __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 ;)
>>>>
>>>>> 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?
>>>>
>>>> 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)
>>>
>
