On Wed, Jul 19, 2017 at 08:19:02PM -0700, Christopher Li wrote:
> On Wed, Jul 19, 2017 at 3:17 PM, Luc Van Oostenryck
> <luc.vanoostenryck@xxxxxxxxx> wrote:
> >
> > Even now, I'm not really back. Just taking time to reply here
> > while I have the opportunity to.
>
> OK. Good to know then guessing
>
>
> >> Can you show me some C input file that your original patch will do the
> >> right thing and current one will miss the opportunity to simplify?
> >> Even for the case it is just a suspect of producing worse code is fine.
> >> Just point me to some test case, I will investigate and compare the
> >> results.
> >
> > Test cases are test cases and code is code.
> >
> > I'll copy here verbatim the commit message of the patch that was fixed
> > by the original patch (the one that had "Fixes: 51cfbc90a5e1462fcd624a1598ecd985a508a5d6
>
> I still don't have a valid test case to difference sparse-next
> and your patch series.
You don't need a test case to *think* about the code or simply read
the explanation I wrote here under.
> If your test case is that switch statement, I make a test case
> out of it. Sparse-next and yoru patch show the same result
> bytecode wise:
Of course, it does.
> > commit 51cfbc90a5e1462fcd624a1598ecd985a508a5d6
> > Author: Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx>
> > Date: 2017-04-26 12:21:21 +0200
> >
> > fix: kill unreachable BBs after killing a child
> >
> > When simplifying a switch into a simple branch all the now
> > unused children of the current BB must be removed.
> > If one of these children become now orphaned, it is directly
> > killed (it will need to be killed soon or later since it is
> > unreachable).
> >
> > However, if one of the killed children is the header of a loop
> > where some variables are updated this may cause problems.
> > Indeed, by killing the header (which contains the phisrc of
> > the entry value of the variable) the whole loop may become
> > unreachable but is not killed yet, OTOH simplification of
> > the associated OP_PHI may create a cycle which may then be
> > detected later by simplify_one_memop() which will issue a
> > "crazy programmer" warning while the programmer was innocent.
>
> That sounds very much like the wine dead loop case recently report on
> the mailing list.
These two cases are not at all related.
> > This situation can be seen in code like:
> > int *p;
> > switch (i - i) { // will be optimized to 0
> > case 0: // will be the simple branch
> > return 0;
> > case 1: // will be optimized away
> > p = ptr;
> > do { // will be an unreachable loop
> > *p++ = 123;
> > } while (--i);
> > }
> >
> > Fix this by calling kill_unreachable_bbs() after having
> > simplified the switch into a branch. This will avoid to
> > create a cycle with because of the removed phisrc in the
> > header and as an added benefit will avoid to waste time
> > trying to simplify BBs that are unreachable.
> >
> > In addition, it's now useless to call kill_bb() for each
> > removed switch's children as kill_unreachable_bbs() will
> > do that too.
>
> The simple patch in sparse-next has the same output result
>
> > It's not at all a problem of some missing optimization opportunity.
> > It's a problem of applying some optimization (OP_PHI simplification)
> > and then giving a warning for the consequence of this optimization
> > (the "crazy programmer" warning) in incorrect conditions/wrong
> > assumption (a variable must not be considered as undefined if it
> > is part of an unreachable BB, it should just be ignored).
> >
> > I hope it is clear enough now.
>
> Can you show me a test C code trigger the problem?
OK, it wasn't clear enough.
It will be a few more days before I can access to my usual dev stuff
where my tests and tests results are.
However, using the description here above, it's not very hard to imagine
a situation where the two patch will behave differently.
You can even create a test case from the old test case here above
and removing a single line, the one "p = ptr;". It's also very easy
to create all sort of variants of it.
-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
