Bug 151793 - Polymorphic operands in operators coerces downstream values to double.
Summary: Polymorphic operands in operators coerces downstream values to double.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on: 152433 157890
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-02 21:51 PST by Mark Lam
Modified: 2016-05-19 05:02 PDT (History)
5 users (show)

See Also:


Attachments
work in progress (23.35 KB, patch)
2016-05-06 15:46 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
wow that was easy (28.41 KB, patch)
2016-05-07 13:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
add more things (45.63 KB, patch)
2016-05-07 15:40 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more... (66.58 KB, patch)
2016-05-08 12:36 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
omg starting to work (74.70 KB, patch)
2016-05-08 13:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
passing some tests (76.43 KB, patch)
2016-05-08 14:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
performance (76.83 KB, text/plain)
2016-05-09 10:57 PDT, Filip Pizlo
no flags Details
works on 64-bit (95.18 KB, patch)
2016-05-09 11:04 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (101.57 KB, patch)
2016-05-09 11:44 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (102.42 KB, patch)
2016-05-09 11:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (102.77 KB, patch)
2016-05-09 13:14 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (102.86 KB, patch)
2016-05-09 13:38 PDT, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (103.42 KB, patch)
2016-05-09 16:35 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
performance (78.58 KB, text/plain)
2016-05-09 18:57 PDT, Filip Pizlo
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-12-02 21:51:19 PST
With the addition of support for polymorphic operands to operators in the DFG and FTL, the PredictionPropagationPhase may now coerce downstream math operations to use double type.  This may result in performance pathologies.  We should investigate a way to mitigate this.
Comment 1 Mark Lam 2015-12-02 21:55:21 PST
For some context, see https://bugs.webkit.org/show_bug.cgi?id=151746#c15.
Comment 2 Radar WebKit Bug Importer 2015-12-02 21:55:59 PST
<rdar://problem/23737908>
Comment 3 Filip Pizlo 2016-05-06 15:15:10 PDT
First we need to unbreak the result profiling.  This basically means rolling out http://trac.webkit.org/changeset/200502.
Comment 4 Filip Pizlo 2016-05-06 15:46:39 PDT
Created attachment 278283 [details]
work in progress
Comment 5 Filip Pizlo 2016-05-07 13:29:37 PDT
Created attachment 278336 [details]
wow that was easy

Turns out that the only thing broken with ResultProfile was how ByteCodeParser handled it. It completely skipped the ResultProfile if we were not "likely to take slow case".
Comment 6 Filip Pizlo 2016-05-07 15:40:39 PDT
Created attachment 278344 [details]
add more things
Comment 7 Filip Pizlo 2016-05-08 12:36:24 PDT
Created attachment 278368 [details]
more...

I have to get the slow paths of baseline add/sub/mul to do profiling.
Comment 8 Filip Pizlo 2016-05-08 13:38:01 PDT
Created attachment 278369 [details]
omg starting to work
Comment 9 Filip Pizlo 2016-05-08 14:08:14 PDT
Created attachment 278372 [details]
passing some tests
Comment 10 Filip Pizlo 2016-05-09 10:57:54 PDT
Created attachment 278411 [details]
performance
Comment 11 Filip Pizlo 2016-05-09 11:04:51 PDT
Created attachment 278412 [details]
works on 64-bit

Now I need to port this to 32-bit.  It won't be too hard.
Comment 12 Filip Pizlo 2016-05-09 11:44:03 PDT
Created attachment 278416 [details]
the patch
Comment 13 WebKit Commit Bot 2016-05-09 11:46:23 PDT
Attachment 278416 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Filip Pizlo 2016-05-09 11:49:38 PDT
Comment on attachment 278416 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278416&action=review

> Source/JavaScriptCore/bytecode/DFGExitProfile.cpp:47
> +    if (false) // FIXME

Ooops.  I meant to add an Option for this.  I just added a Options::verboseExitProfile() locally.
Comment 15 Filip Pizlo 2016-05-09 11:55:09 PDT
Created attachment 278418 [details]
the patch

Turns out I should make TagRegistersMode.h a private header.
Comment 16 WebKit Commit Bot 2016-05-09 11:57:28 PDT
Attachment 278418 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Mark Lam 2016-05-09 12:31:18 PDT
I counted 7 "definitely slower"s in the perf results.  Are those real, or just due to noise?
Comment 18 Mark Lam 2016-05-09 12:33:10 PDT
Looks like some WES bot sadness too.  The EFL one is definitely fixable.
Comment 19 Filip Pizlo 2016-05-09 12:34:54 PDT
(In reply to comment #17)
> I counted 7 "definitely slower"s in the perf results.  Are those real, or
> just due to noise?

I don't know.  We usually ignore individual benchmark results if they contradict the aggregate scores.  JSRegress's aggregate is definitely faster.  The other benchmark suites' aggregate scores did not change.  So, I think we don't need to pay close attention to individual benchmarks.

Note that if you're using 95% confidence intervals as we are, then you should see 1/20 benchmarks report a "definitely" result even if you're comparing a build to itself.  That's why it's good to err on the side of ignoring individual benchmarks unless they report a big change or the benchmark aggregate reports a change.
Comment 20 Filip Pizlo 2016-05-09 13:13:21 PDT
Looks like the test failures that the bots saw were not real.  I've fixed the EFL build failure, that actually revealed a bug!
Comment 21 Filip Pizlo 2016-05-09 13:14:11 PDT
Created attachment 278431 [details]
the patch

Fixes a compile bug in the baseline op_sub impl
Comment 22 WebKit Commit Bot 2016-05-09 13:16:07 PDT
Attachment 278431 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Filip Pizlo 2016-05-09 13:38:41 PDT
Created attachment 278435 [details]
the patch

Let's try this again!
Comment 24 WebKit Commit Bot 2016-05-09 13:41:57 PDT
Attachment 278435 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Mark Lam 2016-05-09 14:09:58 PDT
Comment on attachment 278418 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278418&action=review

> Source/JavaScriptCore/bytecode/ValueProfile.h:255
> +    void emitDetectBitsLight(CCallHelpers&, JSValueRegs, TagRegistersMode = HaveTagRegisters);
> +    
> +    void detectBitsLight(JSValue value)

Why the "Light"?  emitDetectBitsLight seems ambiguous and doesn't actually describe its purpose / action.  How about renaming emitDetectBitsLight as emitDetectNumericness, and  detectBitsLight as detectNumericness?  Unfortunately, I don't have a better word for Numericness, but I think it does accurately convey the idea of what's being done by these functions.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:239
> -            if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())
> -                && m_graph.hasExitSite(node->origin.semantic, BadType)) {
> +            if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())) {

Why not do the same removal of "m_graph.hasExitSite(node->origin.semantic, BadType)" for ArithSub as well?  I only mention ArithSub because you worked on the sub operator in this patch.  Note: the Bit??? ops, and ArithDiv/Mod also has this potentially unneeded check.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:781
> +    Jump branchIfNotDoubleKnownNotInt32(JSValueRegs regs, TagRegistersMode mode = HaveTagRegisters)

It looks like what this code is branching if the regs contains a non-numeric JSValue.  Why not name this emitter method branchIfNotNumber instead?  I see that there is already a branchIfNotNumber() and its 64-bit implementation is identical to this one.  Ah, based on the 32-bit impl, I see what you mean by "IfNotDouble" and "KnownNotInt32".

If this is emitter is not crucial to perf, I suggest just removing this and just using branchIfNotNumber instead.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:991
> +    ResultProfile* resultProfile = nullptr;
> +    if (shouldEmitProfiling())
> +        resultProfile = m_codeBlock->ensureResultProfile(m_bytecodeOffset);

You forgot to plug the resultProfile into the JITSubGenerator below.  You're currently not detecting double results on the fast path.
Comment 26 Filip Pizlo 2016-05-09 14:21:24 PDT
(In reply to comment #25)
> Comment on attachment 278418 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278418&action=review
> 
> > Source/JavaScriptCore/bytecode/ValueProfile.h:255
> > +    void emitDetectBitsLight(CCallHelpers&, JSValueRegs, TagRegistersMode = HaveTagRegisters);
> > +    
> > +    void detectBitsLight(JSValue value)
> 
> Why the "Light"?  emitDetectBitsLight seems ambiguous and doesn't actually
> describe its purpose / action.  How about renaming emitDetectBitsLight as
> emitDetectNumericness, and  detectBitsLight as detectNumericness? 
> Unfortunately, I don't have a better word for Numericness, but I think it
> does accurately convey the idea of what's being done by these functions.

I like emitDetectNumericness and detectNumericness.  I will use that.

> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:239
> > -            if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())
> > -                && m_graph.hasExitSite(node->origin.semantic, BadType)) {
> > +            if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())) {
> 
> Why not do the same removal of "m_graph.hasExitSite(node->origin.semantic,
> BadType)" for ArithSub as well?  I only mention ArithSub because you worked
> on the sub operator in this patch.  Note: the Bit??? ops, and ArithDiv/Mod
> also has this potentially unneeded check.

Oh wow!  I will fix this for ArithSub.

This patch doesn't do anything for div/mod, so I will leave those unchanged.  I think that we should leave div/mod for after this lands and stabilises.

> 
> > Source/JavaScriptCore/jit/AssemblyHelpers.h:781
> > +    Jump branchIfNotDoubleKnownNotInt32(JSValueRegs regs, TagRegistersMode mode = HaveTagRegisters)
> 
> It looks like what this code is branching if the regs contains a non-numeric
> JSValue.  Why not name this emitter method branchIfNotNumber instead?  I see
> that there is already a branchIfNotNumber() and its 64-bit implementation is
> identical to this one.  Ah, based on the 32-bit impl, I see what you mean by
> "IfNotDouble" and "KnownNotInt32".
> 
> If this is emitter is not crucial to perf, I suggest just removing this and
> just using branchIfNotNumber instead.

It saves a register, which is important in a bunch of places.

> 
> > Source/JavaScriptCore/jit/JITArithmetic.cpp:991
> > +    ResultProfile* resultProfile = nullptr;
> > +    if (shouldEmitProfiling())
> > +        resultProfile = m_codeBlock->ensureResultProfile(m_bytecodeOffset);
> 
> You forgot to plug the resultProfile into the JITSubGenerator below.  You're
> currently not detecting double results on the fast path.

Fixed!
Comment 27 Mark Lam 2016-05-09 14:24:44 PDT
Comment on attachment 278435 [details]
the patch

r=me with fixes, and if perf does not regress after the ArithSub OSR exit check is removed.

You mentioned leaving ArithMod/Div alone.  I presume you'll leave the Bit ops alone too since you didn't touch those in this patch?
Comment 28 Filip Pizlo 2016-05-09 16:34:46 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > Comment on attachment 278418 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278418&action=review
> > 
> > > Source/JavaScriptCore/bytecode/ValueProfile.h:255
> > > +    void emitDetectBitsLight(CCallHelpers&, JSValueRegs, TagRegistersMode = HaveTagRegisters);
> > > +    
> > > +    void detectBitsLight(JSValue value)
> > 
> > Why the "Light"?  emitDetectBitsLight seems ambiguous and doesn't actually
> > describe its purpose / action.  How about renaming emitDetectBitsLight as
> > emitDetectNumericness, and  detectBitsLight as detectNumericness? 
> > Unfortunately, I don't have a better word for Numericness, but I think it
> > does accurately convey the idea of what's being done by these functions.
> 
> I like emitDetectNumericness and detectNumericness.  I will use that.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:239
> > > -            if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())
> > > -                && m_graph.hasExitSite(node->origin.semantic, BadType)) {
> > > +            if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())) {
> > 
> > Why not do the same removal of "m_graph.hasExitSite(node->origin.semantic,
> > BadType)" for ArithSub as well?  I only mention ArithSub because you worked
> > on the sub operator in this patch.  Note: the Bit??? ops, and ArithDiv/Mod
> > also has this potentially unneeded check.
> 
> Oh wow!  I will fix this for ArithSub.
> 
> This patch doesn't do anything for div/mod, so I will leave those unchanged.
> I think that we should leave div/mod for after this lands and stabilises.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/jit/AssemblyHelpers.h:781
> > > +    Jump branchIfNotDoubleKnownNotInt32(JSValueRegs regs, TagRegistersMode mode = HaveTagRegisters)
> > 
> > It looks like what this code is branching if the regs contains a non-numeric
> > JSValue.  Why not name this emitter method branchIfNotNumber instead?  I see
> > that there is already a branchIfNotNumber() and its 64-bit implementation is
> > identical to this one.  Ah, based on the 32-bit impl, I see what you mean by
> > "IfNotDouble" and "KnownNotInt32".
> > 
> > If this is emitter is not crucial to perf, I suggest just removing this and
> > just using branchIfNotNumber instead.
> 
> It saves a register, which is important in a bunch of places.
> 
> > 
> > > Source/JavaScriptCore/jit/JITArithmetic.cpp:991
> > > +    ResultProfile* resultProfile = nullptr;
> > > +    if (shouldEmitProfiling())
> > > +        resultProfile = m_codeBlock->ensureResultProfile(m_bytecodeOffset);
> > 
> > You forgot to plug the resultProfile into the JITSubGenerator below.  You're
> > currently not detecting double results on the fast path.
> 
> Fixed!
Comment 29 Filip Pizlo 2016-05-09 16:35:47 PDT
Created attachment 278457 [details]
patch for landing

I still need to test performance.  Putting up for EWS.
Comment 30 WebKit Commit Bot 2016-05-09 16:37:34 PDT
Attachment 278457 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Filip Pizlo 2016-05-09 18:57:37 PDT
Created attachment 278470 [details]
performance
Comment 32 Filip Pizlo 2016-05-09 19:02:50 PDT
Landed in http://trac.webkit.org/changeset/200606
Comment 34 Filip Pizlo 2016-05-09 21:50:44 PDT
(In reply to comment #33)
> This broke the CLoop build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100

Looking!
Comment 35 Filip Pizlo 2016-05-09 21:52:28 PDT
(In reply to comment #34)
> (In reply to comment #33)
> > This broke the CLoop build:
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100
> 
> Looking!

I have a fix, will land shortly.
Comment 36 Filip Pizlo 2016-05-09 21:58:12 PDT
(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > This broke the CLoop build:
> > > https://build.webkit.org/builders/
> > > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100
> > 
> > Looking!
> 
> I have a fix, will land shortly.

Fixed in http://trac.webkit.org/changeset/200613.
Comment 37 Chris Dumez 2016-05-11 09:34:07 PDT
Looks like a 1.7% on Dromaeo DOM Core benchmark on MacBookAir.
Comment 38 Chris Dumez 2016-05-11 09:35:59 PDT
(In reply to comment #37)
> Looks like a 1.7% on Dromaeo DOM Core benchmark on MacBookAir.

I forgot the *key* word: 1.7% *PROGRESSION*!