Current implementation of Register usage optimization in mathIC compilation aren't with the desired logic because it is not updating JITMathIC operands before check if they are constant.
Created attachment 285902 [details] Patch
Comment on attachment 285902 [details] Patch Attachment 285902 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1857047 Number of test failures exceeded the failure limit.
Created attachment 285904 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 285902 [details] Patch Attachment 285902 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1857051 Number of test failures exceeded the failure limit.
Created attachment 285905 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 285902 [details] Patch Attachment 285902 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1857062 Number of test failures exceeded the failure limit.
Created attachment 285906 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 285902 [details] Patch Attachment 285902 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1857084 Number of test failures exceeded the failure limit.
Created attachment 285907 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 285914 [details] Patch
Comment on attachment 285914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285914&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3474 > + mathIC->setLeftOperand(leftOperand); > + mathIC->setRightOperand(rightOperand); > + Thanks for identifying the issue. A few points come to mind: 1. I think this is brittle and hacky because you're setting the SnippetOperands here and then overriding it again later in the generator initialization. However, I'm not entirely satisfied with any alternatives that I can think of at the moment either (need more consideration). At minimum, we should have a debug build flag in the Generator that says that it isn't (or its snippetOperands aren't) initialized yet, and assert on that flag in JITMathIC::isLeftOperandValidConstant() and isRightOperandValidConstant(). 2. Can you add some tests that shows that this is broken (unless existing tests can already cover this)? This will come in handy if someone accidentally breaks this in the future. The assertions suggested in (1) should make it easier to test. Thanks.
Comment on attachment 285914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285914&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3474 >> + > > Thanks for identifying the issue. A few points come to mind: > > 1. I think this is brittle and hacky because you're setting the SnippetOperands here and then overriding it again later in the generator initialization. However, I'm not entirely satisfied with any alternatives that I can think of at the moment either (need more consideration). At minimum, we should have a debug build flag in the Generator that says that it isn't (or its snippetOperands aren't) initialized yet, and assert on that flag in JITMathIC::isLeftOperandValidConstant() and isRightOperandValidConstant(). > > 2. Can you add some tests that shows that this is broken (unless existing tests can already cover this)? This will come in handy if someone accidentally breaks this in the future. The assertions suggested in (1) should make it easier to test. > > Thanks. Caio, I just realized there is a weird dependency here between populating the left/right regs, and creating the Generator. I agree with mark that it seems brittle to set them here and then overwrite them. My suggestion, to a less brittle approach, is to simply have isLeftOperandValidConstant(...) and isRightOperandValidConstant(...) take SnippetOperand as an argument to the function. This won't allow callers to mess this up. You can just make them static functions on the JITBlahGenerator. Also, if you make that change, you'll need to make sure we don't consider left *and* right as constants. We only allow for one or the other being a constant. So that means you'll have to change the if statement below to be an else if Mark, to address your point (2), this was just removing an optimization, it wasn't breaking existing tests AFAIK.
(In reply to comment #12) > Comment on attachment 285914 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285914&action=review > > >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3474 > >> + > > > > Thanks for identifying the issue. A few points come to mind: > > > > 1. I think this is brittle and hacky because you're setting the SnippetOperands here and then overriding it again later in the generator initialization. However, I'm not entirely satisfied with any alternatives that I can think of at the moment either (need more consideration). At minimum, we should have a debug build flag in the Generator that says that it isn't (or its snippetOperands aren't) initialized yet, and assert on that flag in JITMathIC::isLeftOperandValidConstant() and isRightOperandValidConstant(). > > > > 2. Can you add some tests that shows that this is broken (unless existing tests can already cover this)? This will come in handy if someone accidentally breaks this in the future. The assertions suggested in (1) should make it easier to test. > > > > Thanks. > > Caio, I just realized there is a weird dependency here between populating > the left/right regs, and creating the Generator. > I agree with mark that it seems brittle to set them here and then overwrite > them. My suggestion, to a less brittle approach, > is to simply have isLeftOperandValidConstant(...) and > isRightOperandValidConstant(...) take SnippetOperand as an argument > to the function. This won't allow callers to mess this up. You can just make > them static functions on the JITBlahGenerator. Also, if you make that > change, you'll need > to make sure we don't consider left *and* right as constants. We only allow > for one or the other being a constant. So that means you'll have to change > the > if statement below to be an else if I do like this idea of making these methods static and explicitly passing the SnippetOperand to them instead. > Mark, to address your point (2), this was just removing an optimization, it > wasn't breaking existing tests AFAIK. Good point.
(In reply to comment #13) > (In reply to comment #12) > > Comment on attachment 285914 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=285914&action=review > > > > >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3474 > > >> + > > > > > > Thanks for identifying the issue. A few points come to mind: > > > > > > 1. I think this is brittle and hacky because you're setting the SnippetOperands here and then overriding it again later in the generator initialization. However, I'm not entirely satisfied with any alternatives that I can think of at the moment either (need more consideration). At minimum, we should have a debug build flag in the Generator that says that it isn't (or its snippetOperands aren't) initialized yet, and assert on that flag in JITMathIC::isLeftOperandValidConstant() and isRightOperandValidConstant(). > > > > > > 2. Can you add some tests that shows that this is broken (unless existing tests can already cover this)? This will come in handy if someone accidentally breaks this in the future. The assertions suggested in (1) should make it easier to test. > > > > > > Thanks. > > > > Caio, I just realized there is a weird dependency here between populating > > the left/right regs, and creating the Generator. > > I agree with mark that it seems brittle to set them here and then overwrite > > them. My suggestion, to a less brittle approach, > > is to simply have isLeftOperandValidConstant(...) and > > isRightOperandValidConstant(...) take SnippetOperand as an argument > > to the function. This won't allow callers to mess this up. You can just make > > them static functions on the JITBlahGenerator. Also, if you make that > > change, you'll need > > to make sure we don't consider left *and* right as constants. We only allow > > for one or the other being a constant. So that means you'll have to change > > the > > if statement below to be an else if > > I do like this idea of making these methods static and explicitly passing > the SnippetOperand to them instead. Nice. It was one of solutions that I had in mind. > > Mark, to address your point (2), this was just removing an optimization, it > > wasn't breaking existing tests AFAIK. > > Good point. Yes. JSStress/stree/op_add.js op_mull.js handle these cases and also there are some tests in LayoutTests too. We found this bug in https://bugs.webkit.org/show_bug.cgi?id=160012 that I am working.
Created attachment 286001 [details] Patch
Attachment 286001 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:49: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:66: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:71: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:47: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:63: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:68: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:42: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:49: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:67: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:72: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 15 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
It was not correct replace all mathIC->is(operand)ValidConstant() to Generator::is(operand)ValidConstant(SnippetOperando) because in some cases it is breaking some tests. So, I think the static method is a good solution for the DFGSpeculativeJIT case. Another possible solution is also implement setters to left, right, leftRegs and rightRegs.
Created attachment 286002 [details] Patch
Created attachment 286040 [details] Patch
Comment on attachment 286040 [details] Patch Attachment 286040 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1872151 Number of test failures exceeded the failure limit.
Created attachment 286041 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286040 [details] Patch Attachment 286040 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1872162 Number of test failures exceeded the failure limit.
Created attachment 286043 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 286040 [details] Patch Attachment 286040 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1872256 Number of test failures exceeded the failure limit.
Created attachment 286046 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286040 [details] Patch Attachment 286040 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1872260 Number of test failures exceeded the failure limit.
Created attachment 286047 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286049 [details] Patch
Comment on attachment 286049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286049&action=review > Source/JavaScriptCore/ChangeLog:10 > + We created in this Patch isLeftOperandValidConstant(SnippetOperand) and > + isRighttOperandValidConstant(SnippetOperand) in JIT*Generators because > + JSC::DFG::SpeculativeJIT::compileMathIC also need to check This changelog isn't very clear. Can you rewrite this to more clearly state that this patch fixes a broken optimization where we would always populate the lhs/rhs registers even though the snippet could handle either lhs/rhs not being populated because it's a constant. > Source/JavaScriptCore/ChangeLog:13 > + Generator Object. The old version was always returning true > + because mathIC->generator was a fresh allocated object. Was returning true to what? Do you mean each (lhs/rhs)IsvalidConstant were returning false before? > Source/JavaScriptCore/jit/JITArithmetic.cpp:741 > + if (!Generator::isLeftOperandValidConstant(leftOperand)) > emitGetVirtualRegister(op1, leftRegs); > - if (!mathIC->isRightOperandValidConstant()) > + if (!Generator::isRightOperandValidConstant(rightOperand)) This doesn't seem correct. This allows for both lhs and rhs to be constant. I think you want the second condition to be if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand))
Created attachment 286147 [details] Patch
Comment on attachment 286147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286147&action=review r=me with suggestion to fix my incorrect analysis in my previous review > Source/JavaScriptCore/ChangeLog:9 > + a register to LHS or RHS if one of these operands are profiled as valid s/profiled/proven Profiled is not the correct requirement. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476 > + if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) { Sorry, I was totally wrong about this code. We don't need to check Generator::isLeftOperandValidConstant(leftOperand) here since the code above makes sure that not both operands can be constant. Sorry for incorrectly suggesting that before. Instead, what I'd do is just have an assertion above like so (by the other related assertion): ASSERT(!(Generator::isLeftOperandValidConstant(leftOperand) && Generator::isRightOperandValidConstant(rightOperand))); > Source/JavaScriptCore/jit/JITArithmetic.cpp:741 > + if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) Ditto here. I'd do the same thing. > Source/JavaScriptCore/jit/JITArithmetic.cpp:803 > I'd also add the assertion here.
Comment on attachment 286147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286147&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476 >> + if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) { > > Sorry, I was totally wrong about this code. We don't need to check Generator::isLeftOperandValidConstant(leftOperand) here since > the code above makes sure that not both operands can be constant. Sorry for incorrectly suggesting that before. > Instead, what I'd do is just have an assertion above like so (by the other related assertion): > ASSERT(!(Generator::isLeftOperandValidConstant(leftOperand) && Generator::isRightOperandValidConstant(rightOperand))); Actually, IMHO I think it is correct. The code previously is just considering "isInt32Constant" and coincidently our current JITMathICs are just considering Int32Constant as possible constant operand. However, one of them can potentially consider a rule with "isNumberConstant" or even RHS or LHS as always valid constant and "(left|right)Child->isInt32Constant" is not considering it. I think this design better because we leave the valid constant rule in JITBlahGenerator responsibility. Does it make sense to you? >> Source/JavaScriptCore/jit/JITArithmetic.cpp:741 >> + if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) > > Ditto here. I'd do the same thing. Ditto above.
Comment on attachment 286147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286147&action=review >>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476 >>> + if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) { >> >> Sorry, I was totally wrong about this code. We don't need to check Generator::isLeftOperandValidConstant(leftOperand) here since >> the code above makes sure that not both operands can be constant. Sorry for incorrectly suggesting that before. >> Instead, what I'd do is just have an assertion above like so (by the other related assertion): >> ASSERT(!(Generator::isLeftOperandValidConstant(leftOperand) && Generator::isRightOperandValidConstant(rightOperand))); > > Actually, IMHO I think it is correct. The code previously is just considering "isInt32Constant" and coincidently our current JITMathICs are just considering Int32Constant as possible constant operand. However, one of them can potentially consider a rule with "isNumberConstant" or even RHS or LHS as always valid constant and "(left|right)Child->isInt32Constant" is not considering it. I think this design better because we leave the valid constant rule in JITBlahGenerator responsibility. Does it make sense to you? I see your argument. I agree with your sentiment, however, I don't completely agree with your assertion that it's JITBlahGenerator's responsibility at the moment to determine what a valid constant is (I would argue that it's partially its responsibility). Currently, the caller of such functions decides whether or not to put constants into the SnippetOperands. I think it's cleaner for the code to go full in on this assumption, and back it up with assertions. I'm not totally against your argument, but I'm not a huge fan of having the code pretend a condition can hold when it can't. I think it's better to just have the code assert that the condition holds, and if we ever decide to improve how constants flow into JITBlahGenerator, we can remove the assertion.
(In reply to comment #33) > Comment on attachment 286147 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286147&action=review > > >>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476 > >>> + if (Generator::isLeftOperandValidConstant(leftOperand) || !Generator::isRightOperandValidConstant(rightOperand)) { > >> > >> Sorry, I was totally wrong about this code. We don't need to check Generator::isLeftOperandValidConstant(leftOperand) here since > >> the code above makes sure that not both operands can be constant. Sorry for incorrectly suggesting that before. > >> Instead, what I'd do is just have an assertion above like so (by the other related assertion): > >> ASSERT(!(Generator::isLeftOperandValidConstant(leftOperand) && Generator::isRightOperandValidConstant(rightOperand))); > > > > Actually, IMHO I think it is correct. The code previously is just considering "isInt32Constant" and coincidently our current JITMathICs are just considering Int32Constant as possible constant operand. However, one of them can potentially consider a rule with "isNumberConstant" or even RHS or LHS as always valid constant and "(left|right)Child->isInt32Constant" is not considering it. I think this design better because we leave the valid constant rule in JITBlahGenerator responsibility. Does it make sense to you? > > I see your argument. I agree with your sentiment, however, I don't > completely agree with your assertion that it's JITBlahGenerator's > responsibility at the moment to determine what a valid constant is (I would > argue that it's partially its responsibility). Currently, the caller of such > functions decides whether or not to put constants into the SnippetOperands. > I think it's cleaner for the code to go full in on this assumption, and back > it up with assertions. I'm not totally against your argument, but I'm not a > huge fan of having the code pretend a condition can hold when it can't. I > think it's better to just have the code assert that the condition holds, and > if we ever decide to improve how constants flow into JITBlahGenerator, we > can remove the assertion. Agreed
Created attachment 287275 [details] Patch
(In reply to comment #35) > Created attachment 287275 [details] > Patch Patch for landing.
Comment on attachment 287275 [details] Patch Attachment 287275 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1966559 New failing tests: fast/scrolling/ios/scroll-events-back-forward.html
Created attachment 287283 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 287275 [details] Patch Clearing flags on attachment: 287275 Committed r205364: <http://trac.webkit.org/changeset/205364>
All reviewed patches have been landed. Closing bug.