...
Created attachment 284392 [details] WIP WIP with a bunch of data gathering code in it.
Created attachment 284462 [details] patch
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.
Created attachment 284495 [details] patch
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).
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.
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 ||
(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.
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.
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.
Created attachment 284503 [details] patch for landing
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.
Created attachment 284511 [details] benchmarks looks neutral, or maybe a slight win.
landed in: https://trac.webkit.org/changeset/203693