Bug 160108 – op_mul/ArithMul(Untyped,Untyped) should be an IC

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

[Saam Barati]

...

[Saam Barati]

Created attachment 284392 [details]
WIP

WIP with a bunch of data gathering code in it.

[Saam Barati]

Created attachment 284462 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:171:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 17 files


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

[Saam Barati]

Created attachment 284495 [details]
patch

[Mark Lam]

Comment on attachment 284462 [details]
patch

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

> Source/JavaScriptCore/jit/JITMulGenerator.cpp:53
> +    if (lhs.isOnlyNonNumber() && rhs.isOnlyNonNumber())

Shouldn't this be ((lhs.isOnlyNonNumber() || rhs.isOnlyNonNumber())?

I wonder why the op_mul.js tests didn't catch this scenario.  Can you look into adding a test case or a new test file as appropriate?  Oh, I think you'll end up returning JITMathICInlineResult::GenerateFullSnippet below, and that will still work though less efficiently.

> Source/JavaScriptCore/jit/JITMulGenerator.cpp:200
> +    if (!m_arithProfile || !shouldEmitProfiling)

I think this should be (!m_arithProfile && !shouldEmitProfiling).

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:171:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


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

[Saam Barati]

Comment on attachment 284462 [details]
patch

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

>> Source/JavaScriptCore/jit/JITMulGenerator.cpp:53
>> +    if (lhs.isOnlyNonNumber() && rhs.isOnlyNonNumber())
> 
> Shouldn't this be ((lhs.isOnlyNonNumber() || rhs.isOnlyNonNumber())?
> 
> I wonder why the op_mul.js tests didn't catch this scenario.  Can you look into adding a test case or a new test file as appropriate?  Oh, I think you'll end up returning JITMathICInlineResult::GenerateFullSnippet below, and that will still work though less efficiently.

Yes this is tricky. We only degrade generating if both have proven to not be numbers. We are more optimistic if one has been a number, that the other might also be a number. We can play around with this heuristic in the future. It might even be helpful to have counts.

>> Source/JavaScriptCore/jit/JITMulGenerator.cpp:200
>> +    if (!m_arithProfile || !shouldEmitProfiling)
> 
> I think this should be (!m_arithProfile && !shouldEmitProfiling).

No, that's not quite right.
If we don't have an ArithProfile, we don't emit profiling. If shouldEmitProfiling is false, we don't emit profiling.
Hence for ||

[Mark Lam]

(In reply to comment #7)
> Comment on attachment 284462 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284462&action=review
> 
> >> Source/JavaScriptCore/jit/JITMulGenerator.cpp:53
> >> +    if (lhs.isOnlyNonNumber() && rhs.isOnlyNonNumber())
> > 
> > Shouldn't this be ((lhs.isOnlyNonNumber() || rhs.isOnlyNonNumber())?
> > 
> > I wonder why the op_mul.js tests didn't catch this scenario.  Can you look into adding a test case or a new test file as appropriate?  Oh, I think you'll end up returning JITMathICInlineResult::GenerateFullSnippet below, and that will still work though less efficiently.
> 
> Yes this is tricky. We only degrade generating if both have proven to not be
> numbers. We are more optimistic if one has been a number, that the other
> might also be a number. We can play around with this heuristic in the
> future. It might even be helpful to have counts.

OK.  Good point.

> >> Source/JavaScriptCore/jit/JITMulGenerator.cpp:200
> >> +    if (!m_arithProfile || !shouldEmitProfiling)
> > 
> > I think this should be (!m_arithProfile && !shouldEmitProfiling).
> 
> No, that's not quite right.
> If we don't have an ArithProfile, we don't emit profiling. If
> shouldEmitProfiling is false, we don't emit profiling.
> Hence for ||

Oh, you're right.  I had a mistake in my reasoning.

[Mark Lam]

Comment on attachment 284495 [details]
patch

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:11
> +        work over arbitrary IC snippets. This will making adding Div/Sub/Pow in the

typo: /will making/will make/.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1326
> +    other.m_jit = nullptr;

I think you should also set "other.m_fpr = InvalidFPRReg;" similar to the GPRTemporary version.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:3122
> +        if (LIKELY(m_jit))

Should this be "if (LIKELY(m_jit && m_fpr != InvalidFPRReg)" similar to the GPRTemporary version?

Looking at the constructors, I see no way that m_fpr can be invalid unless the allocator fails.  I'm thinking we should have the GPRTemporary and FPRTemporary code behaves consistently, either by adding the invalid check, or asserting that the reg is never invalid at destruction time.

[Saam Barati]

Comment on attachment 284495 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:3122
>> +        if (LIKELY(m_jit))
> 
> Should this be "if (LIKELY(m_jit && m_fpr != InvalidFPRReg)" similar to the GPRTemporary version?
> 
> Looking at the constructors, I see no way that m_fpr can be invalid unless the allocator fails.  I'm thinking we should have the GPRTemporary and FPRTemporary code behaves consistently, either by adding the invalid check, or asserting that the reg is never invalid at destruction time.

Mark and I spoke offline, we're going to probably make GPRTemporary behave more like FPRTemporary in another bug.

[Saam Barati]

Created attachment 284503 [details]
patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:171:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


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

[Saam Barati]

Created attachment 284511 [details]
benchmarks

looks neutral, or maybe a slight win.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/203693