Bug 149713
Summary: | DFG peephole does not have effect on CompareEq / CompareStrictEq because of MovHint | ||
---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> |
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED DUPLICATE | ||
Severity: | Normal | CC: | fpizlo, ggaren, keith_miller, mark.lam, msaboff, saam |
Priority: | P2 | ||
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=149616 |
Yusuke Suzuki
Currently, we have op_jless, op_jgreater etc. But we don't have op_jeq.
Instead we compile the following
function eq(a, b)
{
if (a === b)
return true;
return false;
}
to
eq#CU1c7h:[0x7fb57e6dd268->0x7fb57e46c200, NoneFunctionCall, 17]: 17 m_instructions; 136 bytes; 3 parameter(s); 8 callee register(s); 6 variable(s)
[ 0] enter
[ 1] get_scope loc3
[ 3] mov loc4, loc3
[ 6] eq loc6, arg1, arg2
[ 10] jfalse loc6, 5(->15)
[ 13] ret True(const1)
[ 15] ret False(const2)
So, in DFG, between op_eq and op_jfalse, we have MovHint to store the result of op_eq.
As a result, detectPeepHoleBranch() never detects peep hole for that case.
Attachments | ||
---|---|---|
Add attachment proposed patch, testcase, etc. |
Filip Pizlo
When and if we implement a fix that allows the peephole detection to "skip" the MovHint, we should check how much of a speed-up it is.
If it is not a speed-up, then instead of writing a patch that extends the peephole to skip MovHint, we should write a patch that removes all code for those peepholes that are currently dead because of the MovHint. That peephole code is a pain to maintain and it's possible that it buys us nothing.
Yusuke Suzuki
(In reply to comment #1)
> When and if we implement a fix that allows the peephole detection to "skip"
> the MovHint, we should check how much of a speed-up it is.
>
> If it is not a speed-up, then instead of writing a patch that extends the
> peephole to skip MovHint, we should write a patch that removes all code for
> those peepholes that are currently dead because of the MovHint. That
> peephole code is a pain to maintain and it's possible that it buys us
> nothing.
Right. This peep hole optimization is easily broken because the assumption the optimization rely on is fragile.
If it does not provide significant performance improvement, dropping these code is better.
Saam Barati
Yeah, I remember running into not being able
to get peephole optimization to kick in because
of MovHints (that's why I asked you to upload
a test for that, because I thought you found
a way for it to kick in).
Yusuke Suzuki
*** This bug has been marked as a duplicate of bug 177100 ***