> On Jul 1, 2021, at 9:04 AM, Richard Biener <richard.guent...@gmail.com> wrote:
>
> On Thu, Jul 1, 2021 at 3:45 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>
>>
>>
>>> On Jul 1, 2021, at 1:48 AM, Richard Biener <richard.guent...@gmail.com>
>>> wrote:
>>>
>>> On Wed, Jun 30, 2021 at 9:15 PM Qing Zhao via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>>
>>>>
>>>>> On Jun 30, 2021, at 1:59 PM, Richard Biener <rguent...@suse.de> wrote:
>>>>>
>>>>> On June 30, 2021 8:07:43 PM GMT+02:00, Qing Zhao <qing.z...@oracle.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>> 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:
>>>>>
>>>>> 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...
>>>>
>>>> Please see my other email on the new small testing case without
>>>> -ftrivial-auto-var-init. The same issue in SSA with that testing case
>>>> even without -ftrivial-auto-var-init.
>>>> It looks like an existing bug to me in SSA.
>>>
>>> It's not a bug in SSA, it's at most a missed optimization there.
>>
>> I still think that SSA cannot handle block-scoped variable correctly in this
>> case, it should not move the variable out side of the block scope. -:)
>
> Well, the SSA rewriter simply does not "understand" scopes and thus
> keeps registers live across
> the scope boundaries which results in unnecessary PHI nodes in this
> particular case. That's of
> course because "scopes" no longer exist at this point in the IL, they
> are at most approximated
> by CLOBBERs we put at the end of scopes, but we don't do that for everything.
Then, this looks like a bug to me.
If the scope info is emulated by CLOBBERs in this stage, then compiler should
add CLOBBERs at proper places to
keep the scope info.
Moving a variable out of its live scope seems not a correct transformation.
>
> For
>
> void foo ()
> {
> lab:
> {
> int i;
> i += 1;
> }
> goto lab;
> }
>
> we get
>
> <bb 2> :
> # i_1 = PHI <i_2(D)(0), i_3(2)>
Here, the variable “I” had been moved out of its live scope already.
Seems not correct.
Qing
> lab:
> i_3 = i_1 + 1;
> // predicted unlikely by goto predictor.
> goto <bb 2>; [INV]
>
> but
>
> <bb 2> :
> lab:
> i_3 = i_2(D) + 1;
> // predicted unlikely by goto predictor.
> goto <bb 2>; [INV]
>
> would have been correct as well.
>
> Richard.
>
>>> But
>>> with -ftrivial-auto-var-init it
>>> becomes a correctness issue. I think the idea avoiding the USE of the
>>> variable in .DEFERRED_INIT
>>> and instead passing the init size is a good one and should avoid this
>>> case (hopefully).
>>
>>
>> Agreed, I have made such change yesterday, and this work around the current
>> issue.
>>
>> temp = .DEFERRED_INIT (temp/WITH_SIZE_EXPR(temp), init_type)
>>
>> To:
>>
>> temp = .DEFERRED_INIT (SIZE_of_temp, init_type)
>>
>> Thanks.
>>
>> Qing
>>>
>>> Richard.
>>>
>>>> Let me know if I still miss anything
>>>>
>>>> Qing
>>>>>
>>>>> 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.
>>>>>
>>>>> Richard.
>>>>>
>>>>
>>
