Register usage optimization in mathIC when LHS and RHS are constants isn't configured correctly

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

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.

[Caio Lima]

Created attachment 285902 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Caio Lima]

Created attachment 285914 [details]
Patch

[Mark Lam]

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.

[Saam Barati]

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.

[Mark Lam]

(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.

[Caio Lima]

(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.

[Caio Lima]

Created attachment 286001 [details]
Patch

[WebKit Commit Bot]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 286002 [details]
Patch

[Caio Lima]

Created attachment 286040 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Caio Lima]

Created attachment 286049 [details]
Patch

[Saam Barati]

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))

[Caio Lima]

Created attachment 286147 [details]
Patch

[Saam Barati]

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.

[Caio Lima]

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.

[Saam Barati]

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.

[Caio Lima]

(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

[Caio Lima]

Created attachment 287275 [details]
Patch

[Caio Lima]

(In reply to comment #35)
> Created attachment 287275 [details]
> Patch

Patch for landing.

[Build Bot]

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

[Build Bot]

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

[WebKit Commit Bot]

Comment on attachment 287275 [details]
Patch

Clearing flags on attachment: 287275

Committed r205364: <http://trac.webkit.org/changeset/205364>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.