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.
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:
int *ptr;
int foo(int *p, int i)
{
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);
}
return 1;
}
$ ./test-linearize /tmp/d.c
/tmp/d.c:1:5: warning: symbol 'ptr' was not declared. Should it be static?
/tmp/d.c:2:5: warning: symbol 'foo' was not declared. Should it be static?
foo:
.L0:
<entry-point>
ret.32 $0
You know what I want. I want the test case can show sparse-next
have bug or less optimal than your series. If every thing else is
the same, the one in sparse-next is simpler.
>
> 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. I think you are likely having a bug elsewhere make you
believe that you have to kill instruction there. I just tested, your patches
will do the dead loop on the wine test C file as well. We might talking about
the same bug.
>
> 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?
> I also assure that instead of quickly rewriting someone's patch
> (because you don't like it and think you can do better) it would
> be so much better to simply:
I did.
> - *say* what you don't like in the patch
Might have simpler way.
> - *ask* why things are done like this way
I also did. I repeatedly ask for the test C code that show the bug
if I don't immediately remove the dead code.
> - *ask* if things couldn't be done this other way instead.
Same as above. Show me the code please. I would much rather
looking at the test C code that demo the problem than arguing here.
Chris
--
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
