Bug 149713 - DFG peephole does not have effect on CompareEq / CompareStrictEq because of MovHint
Summary: DFG peephole does not have effect on CompareEq / CompareStrictEq because of M...
Status: RESOLVED DUPLICATE of bug 177100
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-01 09:51 PDT by Yusuke Suzuki
Modified: 2018-03-22 05:02 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-10-01 09:51:49 PDT
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.
Comment 1 Filip Pizlo 2015-10-01 10:06:57 PDT
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.
Comment 2 Yusuke Suzuki 2015-10-01 10:23:49 PDT
(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.
Comment 3 Saam Barati 2015-10-01 12:58:35 PDT
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).
Comment 4 Yusuke Suzuki 2018-03-22 05:02:23 PDT

*** This bug has been marked as a duplicate of bug 177100 ***