On Tue, Mar 5, 2019 at 10:48 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao <qing.z...@oracle.com> wrote:
> >
> > Hi, Richard,
> >
> > > On Mar 4, 2019, at 5:45 AM, Richard Biener <richard.guent...@gmail.com>
> > > wrote:
> > >>
> > >> It looks like DOM fails to visit stmts generated by simplification. Can
> > >> you open a bug report with a testcase?
> > >>
> > >>
> > >> The problem is, It took me quite some time in order to come up with a
> > >> small and independent testcase for this problem,
> > >> a little bit change made the error disappear.
> > >>
> > >> do you have any suggestion on this? or can you give me some hint on how
> > >> to fix this in DOM? then I can try the fix on my side?
> > >
> > > I remember running into similar issues in the past where I tried to
> > > extract temporary nonnull ranges from divisions.
> > > I have there
> > >
> > > @@ -1436,11 +1436,16 @@ dom_opt_dom_walker::before_dom_children
> > > m_avail_exprs_stack->pop_to_marker ();
> > >
> > > edge taken_edge = NULL;
> > > - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > > - {
> > > - evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (gsi),
> > > false);
> > > - taken_edge = this->optimize_stmt (bb, gsi);
> > > - }
> > > + gsi = gsi_start_bb (bb);
> > > + if (!gsi_end_p (gsi))
> > > + while (1)
> > > + {
> > > + evrp_range_analyzer.record_def_ranges_from_stmt (gsi_stmt (gsi),
> > > false);
> > > + taken_edge = this->optimize_stmt (bb, &gsi);
> > > + if (gsi_end_p (gsi))
> > > + break;
> > > + evrp_range_analyzer.record_use_ranges_from_stmt (gsi_stmt (gsi));
> > > + }
> > >
> > > /* Now prepare to process dominated blocks. */
> > > record_edge_info (bb);
> > >
> > > OTOH the issue in your case is that fold emits new stmts before gsi but
> > > the
> > > above loop will never look at them. See tree-ssa-forwprop.c for code how
> > > to deal with this (setting a pass-local flag on stmts visited and walking
> > > back
> > > to unvisited, newly inserted ones). The fold_stmt interface could in
> > > theory
> > > also be extended to insert new stmts on a sequence passed to it so the
> > > caller would be responsible for inserting them into the IL and could then
> > > more easily revisit them (but that's a bigger task).
> > >
> > > So, does the following help?
> >
> > Yes, this change fixed the error in my side, now, in the dumped file for
> > pass dom3:
> >
> > ====
> > Visiting statement:
> > i_49 = _98 > 0 ? k_105 : 0;
> > Meeting
> > [0, 65535]
> > and
> > [0, 0]
> > to
> > [0, 65535]
> > Intersecting
> > [0, 65535]
> > and
> > [0, 65535]
> > to
> > [0, 65535]
> > Optimizing statement i_49 = _98 > 0 ? k_105 : 0;
> > Replaced 'k_105' with variable '_98'
> > gimple_simplified to _152 = MAX_EXPR <_98, 0>;
> > i_49 = _152;
>
> Ah, that looks interesting. From this detail we might be
> able to derive a testcase as well - a GIMPLE one
> eventually because DOM runs quite late. It's also interesting
> to see the inefficient code here (the extra copy), probably
> some known issue with match-and-simplify, I'd have to check.
>
> > Folded to: i_49 = _152;
> > LKUP STMT i_49 = _152
> > ==== ASGN i_49 = _152
> >
> > Visiting statement:
> > _152 = MAX_EXPR <_98, 0>;
> >
> > Visiting statement:
> > i_49 = _152;
> > Intersecting
> > [0, 65535] EQUIVALENCES: { _152 } (1 elements)
> > and
> > [0, 65535]
> > to
> > [0, 65535] EQUIVALENCES: { _152 } (1 elements)
> > ====
> >
> > We can clearly see from the above, all the new stmts generated by fold are
> > visited now.
>
> We can also see that DOMs optimize_stmt code is not executed on the first stmt
> of the folding result (the MAX_EXPR), so the fix can be probably
> amended/simplified
> with that in mind.
>
> > it is also confirmed that the runtime error caused by this bug was gone
> > with this fix.
> >
> > So, what’s the next step for this issue?
> >
> > will you commit this fix to gcc9 and gcc8 (we need it in gcc8)?
>
> I'll see to carve out some cycles trying to find a testcase and amend
> the fix a bit
> and will take care of testing/submitting the fix. Thanks for testing
> that it works
> for your case.
I filed PR89595 with a testcase.
Richard.
> Richard.
>
> > or I can test this fix on my side and commit it to both gcc9 and gcc8?
> >
> > thanks.
> >
> > Qing
> >
> > >
> > > Index: gcc/tree-ssa-dom.c
> > > ===================================================================
> > > --- gcc/tree-ssa-dom.c (revision 269361)
> > > +++ gcc/tree-ssa-dom.c (working copy)
> > > @@ -1482,8 +1482,25 @@ dom_opt_dom_walker::before_dom_children
> > > edge taken_edge = NULL;
> > > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > > {
> > > + gimple_stmt_iterator pgsi = gsi;
> > > + gsi_prev (&pgsi);
> > > evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (gsi), false);
> > > taken_edge = this->optimize_stmt (bb, gsi);
> > > + gimple_stmt_iterator npgsi = gsi;
> > > + gsi_prev (&npgsi);
> > > + /* Walk new stmts eventually inserted by DOM. gsi_stmt (gsi)
> > > itself
> > > + while it may be changed should not have gotten a new definition.
> > > */
> > > + if (gsi_stmt (pgsi) != gsi_stmt (npgsi))
> > > + do
> > > + {
> > > + if (gsi_end_p (pgsi))
> > > + pgsi = gsi_start_bb (bb);
> > > + else
> > > + gsi_next (&pgsi);
> > > + evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (pgsi),
> > > + false);
> > > + }
> > > + while (gsi_stmt (pgsi) != gsi_stmt (gsi));
> > > }
> > >
> > > /* Now prepare to process dominated blocks. */
> > >
> > >
> > > Richard.
> > >
> > >> Thanks a lot.
> > >>
> > >> Qing
> > >>
> > >>
> > >>
> > >> Richard.
> > >>
> > >>
> >
