Re: HELP!! How to inhibit optimizations applied to .DEFERRED_INIT argument?

[Qing Zhao <qing.zhao@oracle.com>]

> On Jun 30, 2021, at 1:59 PM, Richard Biener <rguenther@suse.de> wrote:
> 
> On June 30, 2021 8:07:43 PM GMT+02:00, Qing Zhao <qing.zhao@oracle.com> wrote:
>> 
>> 
>>> On Jun 30, 2021, at 12:36 PM, Richard Biener <rguenther@suse.de>
>> wrote:
>>> 
>>> On June 30, 2021 7:20:18 PM GMT+02:00, Andrew Pinski
>> <pinskia@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:
> 
> But that's obviously not how it behaves
> During into SSA phase since we're inserting a PHI for it - and we're inserting it because of the use in the DEFERED_INIT call. I suppose you need to fiddle with the SSA rewrite and avoid treating the use as a use but only for the purpose of inserting PHIs...
> 
> You might be able to construct a testcase which has a use before the real init where then the optimistic CCP propagation will defeat the DEFERED_INIT otherwise. 
> 
> I'd need to play with the actual patch to find a good solution to this problem. 

Do you want me send you the current patch (4th version) with email? 

Or I have a branch at 

https://gitlab.com/x86-gcc/gcc.git
users/qinzhao/auto-init/v3

(And I am currently try to change the DEFERRED_INIT call from:

temp = .DEFERRED_INIT (temp/WITH_SIZE_EXPR(temp), init_type)

To:

temp = .DEFERRED_INIT (SIZE_of_temp, init_type)

And this solution successfully hide this CCP issue and also simplify the implementation at the same time.)

Qing
> 
> Richard.
> 
>>>>> 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 <rguenther@suse.de>
>>>> wrote:
>>>>>> 
>>>>>> On Wed, 30 Jun 2021, Qing Zhao wrote:
>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On Jun 30, 2021, at 2:46 AM, Richard Biener
>>>> <rguenther@suse.de<mailto:rguenther@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 <rguenther@suse.de<mailto:rguenther@suse.de>>
>>>>>>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
>>>> Nuernberg,
>>>>>>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>>> --
>>>>>> Richard Biener <rguenther@suse.de>
>>>>>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
>>>> Nuernberg,
>>>>>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
>>>>> 
>>> 
>