Bug 160073 - [B3] Fusing immediates into test instructions should work again
Summary: [B3] Fusing immediates into test instructions should work again
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-21 23:05 PDT by Filip Pizlo
Modified: 2016-08-01 16:23 PDT (History)
6 users (show)

See Also:


Attachments
I don't know if I'm doing this right (19.23 KB, patch)
2016-07-22 00:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (26.83 KB, patch)
2016-07-22 15:20 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (25.04 KB, patch)
2016-07-22 21:42 PDT, Filip Pizlo
sam: review+
Details | Formatted Diff | Diff
performance (80.42 KB, text/plain)
2016-07-22 21:46 PDT, Filip Pizlo
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-07-21 23:05:37 PDT
When we introduced BitImm, we forgot to change the Branch(BitAnd(value, constant)) fusion.  This emits test instructions, so it should use BitImm for the constant.  But it was still using Imm!
Comment 1 Filip Pizlo 2016-07-22 00:29:08 PDT
Created attachment 284312 [details]
I don't know if I'm doing this right

The logic for deciding what sorts of immediates can be used for Branch(BitAnd) fusion was broken pretty badly.  I'm trying to fix it.
Comment 2 Filip Pizlo 2016-07-22 15:20:23 PDT
Created attachment 284378 [details]
the patch

NOTE: I haven't yet had a chance to perf test this latest version, which is why it's not r? yet.
Comment 3 Filip Pizlo 2016-07-22 21:42:17 PDT
Created attachment 284400 [details]
the patch
Comment 4 Filip Pizlo 2016-07-22 21:46:22 PDT
Created attachment 284401 [details]
performance

It's neutral, except on LongSpider, where it's a regression on math-cordic!  I investigated this regression in detail.  It's hilarious.  The benchmark is very small, so it's sensitive to weird stuff.  And how weird this is.

Previously, there was code like this:

      0x4ea9fda0347e: test %edx, %r10d

Where %r10 is initialized to a constant, which gets hoisted.  Normally this would be bad codegen.  And this patch fixes it to be:

      0x365b91a03457: test $0x7fffffff, %edx

But by doing this, we perturb regalloc throughout the entire function.  Lots of things change in subtle ways.  None of the other effects look, on the surface, like either good or bad.  But in this case, occupying %r10 and so perturbing everything is worth a 9% speed-up.

To me this means that we should ignore the math-cordic regression.  The reason why we're seeing such a weird effect on this test is that this test contains only one hot function.  This function doesn't even do a particularly good job of staying in FTL anyway.
Comment 5 Filip Pizlo 2016-07-24 10:38:13 PDT
Landed in https://trac.webkit.org/changeset/203666
Comment 6 Filip Pizlo 2016-07-24 12:01:26 PDT
Follow-up fix: https://trac.webkit.org/changeset/203668

Late in the game I removed the Imm->BitImm changes in a few places to recover perf regressions, and I went too far by removing it from BranchTest8.

Also, I forgot to spin the load-branch fusion tests on not-x86.
Comment 7 Saam Barati 2016-07-26 19:34:38 PDT
Rolled out in:
http://trac.webkit.org/changeset/203758
Comment 8 Filip Pizlo 2016-08-01 16:23:52 PDT
Relanded in https://trac.webkit.org/changeset/203996