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.
For
void foo ()
{
lab:
{
int i;
i += 1;
}
goto lab;
}
we get
<bb 2> :
# i_1 = PHI <i_2(D)(0), i_3(2)>
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.
> >>>
> >>
>
