Bug 151746 – Polymorphic operand types for DFG and FTL mul.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/23725093>

[Mark Lam]

*** Bug 151748 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 266484 [details]
proposed patch.

[WebKit Commit Bot]

Attachment 266484 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 266485 [details]
x86_64 benchmark result.

[Mark Lam]

Created attachment 266486 [details]
x86 benchmark result.

[Mark Lam]

Created attachment 266487 [details]
x86_64 benchmark result without FTL.

[Mark Lam]

Benchmark results are neutral.  The newly added ftp-object-mul.js test shows the benefit of being able to stay in the DFG and FTL compiled function instead of OSR exiting back to the baseline JIT.

This patch has passed the layout tests and JSC tests.

[Mark Lam]

Created attachment 266507 [details]
patch 2: fixed a value in the test, and a bit more clean up in the DFG mul code.

[WebKit Commit Bot]

Attachment 266507 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 266508 [details]
patch 3: a bit more code clean up in the FTL code.

[WebKit Commit Bot]

Attachment 266508 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 266510 [details]
patch 4: update one more test value in op_mul.js.

[WebKit Commit Bot]

Attachment 266510 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:40:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 266510 [details]
patch 4: update one more test value in op_mul.js.

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

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:346
> +                    changed |= mergePrediction(SpecInt32 | SpecBytecodeDouble);

Do you have a bug for improving this?  I think that this can easily create performance pathologies, since it will cause all downstream math operations on this value to coerce it to double.

[Filip Pizlo]

Comment on attachment 266510 [details]
patch 4: update one more test value in op_mul.js.

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3449
> +        auto& leftChild = node->child1();

I think it's more clear to say Edge instead of auto.

[Mark Lam]

Thanks for the review.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3449
> > +        auto& leftChild = node->child1();
> 
> I think it's more clear to say Edge instead of auto.

I've changed all auto& of type Edge& in my patch to explicitly say Edge& instead.

> > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:346
> > +                    changed |= mergePrediction(SpecInt32 | SpecBytecodeDouble);
> 
> Do you have a bug for improving this?  I think that this can easily create
> performance pathologies, since it will cause all downstream math operations
> on this value to coerce it to double.

I just filed https://bugs.webkit.org/show_bug.cgi?id=151793 to track this.  My thinking on this is that this coercion is supposed to only happen if we encounter polymorphic types, which is supposedly rare.  Hence, the impact of this coercion should be similarly rare.  But we can investigate this some more later.  So far, the benchmarks don't seem to be affected by it (as expected).

Landed in r192993: <http://trac.webkit.org/r192993>.