Bug 160802 - Register usage optimization in mathIC when LHS and RHS are constants isn't configured correctly
Summary: Register usage optimization in mathIC when LHS and RHS are constants isn't co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 160012
  Show dependency treegraph
 
Reported: 2016-08-12 01:52 PDT by Caio Lima
Modified: 2016-09-02 12:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.33 KB, patch)
2016-08-12 02:01 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (793.47 KB, application/zip)
2016-08-12 02:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (808.05 KB, application/zip)
2016-08-12 02:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.17 MB, application/zip)
2016-08-12 02:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (839.52 KB, application/zip)
2016-08-12 03:08 PDT, Build Bot
no flags Details
Patch (6.81 KB, patch)
2016-08-12 09:38 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (10.90 KB, patch)
2016-08-13 00:41 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2016-08-13 01:34 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (9.86 KB, patch)
2016-08-14 17:01 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.48 MB, application/zip)
2016-08-14 17:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.63 MB, application/zip)
2016-08-14 17:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (962.22 KB, application/zip)
2016-08-14 18:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (2.05 MB, application/zip)
2016-08-14 18:14 PDT, Build Bot
no flags Details
Patch (10.58 KB, patch)
2016-08-14 23:29 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2016-08-15 21:04 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (11.38 KB, patch)
2016-08-29 09:41 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (4.10 MB, application/zip)
2016-08-29 11:13 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2016-08-12 01:52:08 PDT
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.
Comment 1 Caio Lima 2016-08-12 02:01:57 PDT
Created attachment 285902 [details]
Patch
Comment 2 Build Bot 2016-08-12 02:39:59 PDT
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.
Comment 3 Build Bot 2016-08-12 02:40:02 PDT
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 4 Build Bot 2016-08-12 02:42:09 PDT
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.
Comment 5 Build Bot 2016-08-12 02:42:11 PDT
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 6 Build Bot 2016-08-12 02:52:54 PDT
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.
Comment 7 Build Bot 2016-08-12 02:52:58 PDT
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 8 Build Bot 2016-08-12 03:08:23 PDT
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.
Comment 9 Build Bot 2016-08-12 03:08:27 PDT
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
Comment 10 Caio Lima 2016-08-12 09:38:47 PDT
Created attachment 285914 [details]
Patch
Comment 11 Mark Lam 2016-08-12 10:11:35 PDT
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 12 Saam Barati 2016-08-12 11:22:03 PDT
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.
Comment 13 Mark Lam 2016-08-12 11:26:12 PDT
(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.
Comment 14 Caio Lima 2016-08-12 21:48:32 PDT
(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.
Comment 15 Caio Lima 2016-08-13 00:41:47 PDT
Created attachment 286001 [details]
Patch
Comment 16 WebKit Commit Bot 2016-08-13 00:43:38 PDT
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.
Comment 17 Caio Lima 2016-08-13 00:49:42 PDT
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.
Comment 18 Caio Lima 2016-08-13 01:34:24 PDT
Created attachment 286002 [details]
Patch
Comment 19 Caio Lima 2016-08-14 17:01:53 PDT
Created attachment 286040 [details]
Patch
Comment 20 Build Bot 2016-08-14 17:31:42 PDT
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.
Comment 21 Build Bot 2016-08-14 17:31:45 PDT
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 22 Build Bot 2016-08-14 17:36:04 PDT
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.
Comment 23 Build Bot 2016-08-14 17:36:08 PDT
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 24 Build Bot 2016-08-14 18:06:42 PDT
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.
Comment 25 Build Bot 2016-08-14 18:06:45 PDT
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 26 Build Bot 2016-08-14 18:14:04 PDT
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.
Comment 27 Build Bot 2016-08-14 18:14:08 PDT
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
Comment 28 Caio Lima 2016-08-14 23:29:12 PDT
Created attachment 286049 [details]
Patch
Comment 29 Saam Barati 2016-08-15 17:03:00 PDT
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))
Comment 30 Caio Lima 2016-08-15 21:04:43 PDT
Created attachment 286147 [details]
Patch
Comment 31 Saam Barati 2016-08-24 18:29:28 PDT
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 32 Caio Lima 2016-08-24 23:29:10 PDT
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 33 Saam Barati 2016-08-28 23:37:29 PDT
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.
Comment 34 Caio Lima 2016-08-29 08:38:49 PDT
(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
Comment 35 Caio Lima 2016-08-29 09:41:57 PDT
Created attachment 287275 [details]
Patch
Comment 36 Caio Lima 2016-08-29 09:42:49 PDT
(In reply to comment #35)
> Created attachment 287275 [details]
> Patch

Patch for landing.
Comment 37 Build Bot 2016-08-29 11:13:06 PDT
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
Comment 38 Build Bot 2016-08-29 11:13:12 PDT
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 39 WebKit Commit Bot 2016-09-02 12:51:03 PDT
Comment on attachment 287275 [details]
Patch

Clearing flags on attachment: 287275

Committed r205364: <http://trac.webkit.org/changeset/205364>
Comment 40 WebKit Commit Bot 2016-09-02 12:51:11 PDT
All reviewed patches have been landed.  Closing bug.