Bug 162124 – op_get_by_id_with_this should use inline caching

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

[Caio Lima]

The current implementation is using the instruction op_get_by_id_with_this to handle super getter bindings correctly. This operand is just a call to slow path which means that getters called from super aren't optimized to use IC or accessor inlining.

To unify these opcodes, we need to add one more operand on op_get_by_id to store the this reference and teach the following layers and ICs that base and this operands sometimes aren't the same.

[Caio Lima]

Created attachment 289188 [details]
WIP - merge get_by_id in all layers

This Patch is implementing get_by_id with new this operand in all layers.

However I am facing a problem on LLInt layer with the following sample:

```
class A {
    get a() {throw "Bad getter";}
}

let o = new A();
o.a
```

The problem here is when the getter throws an exception. The dispatch operation to the exception just fails when I change op_get_by_id dispatch to 10 (the current is 9), and I'm getting EXEC_BAD_ACCESS as error. I am investigating into this. Do I need to configure some other place to change op_get_by_id length to 10?

[Caio Lima]

Created attachment 289213 [details]
WIP - merge op_get_by_id

This is the version almost finished. Lets see what robots think about it.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/InlineAccess.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:47:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:77:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:79:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:140:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:363:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:364:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:365:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:366:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:367:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:368:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:69:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:112:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 16 in 29 files


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

[Build Bot]

Comment on attachment 289213 [details]
WIP - merge op_get_by_id

Attachment 289213 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2103402

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-forms.html

[Build Bot]

Created attachment 289214 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289213 [details]
WIP - merge op_get_by_id

Attachment 289213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2103410

New failing tests:
js/dfg-constant-fold-logical-not-branch.html
imported/w3c/web-platform-tests/url/a-element-xhtml.xhtml

[Build Bot]

Created attachment 289215 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 289220 [details]
WIP - merge op_get_by_id

Ok. Now changed the patch to compile on iOS and fix the errors from before.
To finish I need to remove op_get_by_id_with_this code and support CustomGetter case.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1554:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:77:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:79:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:140:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:363:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:364:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:365:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:366:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:367:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:368:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:69:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:112:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:187:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 16 in 28 files


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

[Caio Lima]

Created attachment 289331 [details]
Benchmarks results

It seems that the current Patch keeps benchmarks neutral and over ~20% improvements in super-get-by-id micro benchmarks:

   super-get-by-id-with-this-monomorphic             36.5942+-0.7969     ^     29.8105+-0.3676        ^ definitely 1.2276x faster
   super-get-by-id-with-this-polymorphic             35.1846+-2.1380     ^     28.1864+-0.6060        ^ definitely 1.2483x faster

General results:
Geomean of preferred means:
   <scaled-result>                                   27.6482+-0.1572           27.5743+-0.1447          might be 1.0027x faster

I suspect we can improve it a little more in compilation phase, since it can be possible remove if(m_base->isSuperNode()).

[Caio Lima]

Created attachment 289377 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:77:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:79:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:140:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:363:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:364:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:365:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:366:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:367:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:368:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:69:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:112:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 31 files


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

[Geoffrey Garen]

Comment on attachment 289377 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        This patch is merging the op_get_by_id_with_this with op_get_by_id and
> +        this way we enable get_by_id optimizations such as Monomorphic/Polymorphic Inline
> +        Cache on JIT layers for super member access. These optimizations is improving access
> +        of super members in ~20%.

Fully unoptimized property access is 10X slower than fully optimized property access. Therefore, it's surprising that this patch is not a bigger win on super-get-by-id-with-this-monomorphic. It looks like super-get-by-id-with-this-monomorphic is a getter/setter benchmark. Can you verify that the FTL successfully inlines the getter and setter for value()? Perhaps the win on this benchmark is not bigger because the benchmark includes an intermediate "calc" function, which is pretty expensive. Can you report the speedup on this benchmark if you remove the call to calc?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1096
> -            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForGetGPR;
> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;

Isn't the answer always thisGPR? Under what conditions do I want to invoke a getter or setter without the specified this?

[Caio Lima]

(In reply to comment #13)
> Comment on attachment 289377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289377&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        This patch is merging the op_get_by_id_with_this with op_get_by_id and
> > +        this way we enable get_by_id optimizations such as Monomorphic/Polymorphic Inline
> > +        Cache on JIT layers for super member access. These optimizations is improving access
> > +        of super members in ~20%.
> 
> Fully unoptimized property access is 10X slower than fully optimized
> property access. Therefore, it's surprising that this patch is not a bigger
> win on super-get-by-id-with-this-monomorphic. It looks like
> super-get-by-id-with-this-monomorphic is a getter/setter benchmark. Can you
> verify that the FTL successfully inlines the getter and setter for value()?
> Perhaps the win on this benchmark is not bigger because the benchmark
> includes an intermediate "calc" function, which is pretty expensive. Can you
> report the speedup on this benchmark if you remove the call to calc?

Good to know that. I faced a problem while creating the patch to get GetById compiled in FTL layer. To get it working, I was using --accessInline=false. What I mean is that the reason can br related with that. I am going to investigate it.

> > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1096
> > -            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForGetGPR;
> > +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
> 
> Isn't the answer always thisGPR? Under what conditions do I want to invoke a
> getter or setter without the specified this?

If I am not wrong, no. This code is dominated by this manipulation over baseForGetGPR (https://github.com/caiolima/webkit/blob/merge_by_ids/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp#L879). So, when thisGPR == baseGPR, it is safer get baseForGetGPR because of "Proxy observed" lookups. I am thinking that it is worth think in cases when thisOperand != baseOperand and it is "Proxy Observed". Does it make sense?

[Caio Lima]

(In reply to comment #13)
> Comment on attachment 289377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289377&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        This patch is merging the op_get_by_id_with_this with op_get_by_id and
> > +        this way we enable get_by_id optimizations such as Monomorphic/Polymorphic Inline
> > +        Cache on JIT layers for super member access. These optimizations is improving access
> > +        of super members in ~20%.
> 
> Fully unoptimized property access is 10X slower than fully optimized
> property access. Therefore, it's surprising that this patch is not a bigger
> win on super-get-by-id-with-this-monomorphic. It looks like
> super-get-by-id-with-this-monomorphic is a getter/setter benchmark. Can you
> verify that the FTL successfully inlines the getter and setter for value()?
> Perhaps the win on this benchmark is not bigger because the benchmark
> includes an intermediate "calc" function, which is pretty expensive. Can you
> report the speedup on this benchmark if you remove the call to calc?

Removed from benchmark setter, calc and ArrayAccess and got the following report:

super-get-by-id-with-this-monomorphic       16.7063+-1.1934     ^     10.8042+-0.4034        ^ definitely 1.5463x faster

The code running is:
```
class A {
    constructor(x) { this._value = x; }
    set value(x) { this._value = x; }
    get value() { return this._value; }
}
class B extends A {
    set value(x) { super.value = x; }
    get value() { return super.value; }
}

const bench = (init, num) => {
    let arr = [];

    let sum = 0;
    for (let i = 0; i != num; ++i){ 
        let o = new B(init);
        sum += o.value;
    }
};

bench(2, 10000);
bench(1 << 30, 10000);
bench(42.2, 10000);
bench(42.5e10, 10000);
```

I was expecting improvements on getter access cases. When they are inlined, it still calls the getter function, so I suppose the speedup is not in the same order of magnitude of non-getter properties, that is just an ```move``` using the property offset.

I thought 20% was good, because I was using  L. Peter Deutsch and Allan M. Schiffman's work as sand line ("Efficient implementation of the smalltalk-80 system"). On their paper, they reported that "SOAR (a Smalltalk implementation for a RISC processor) would have been 33% slower without inline caching" on page 14. Do you have any reference that evaluates speedups on x86_64 arch?

Also, What do you think about ~50% speedup?

[Caio Lima]

Created attachment 290487 [details]
Patch for landing

[Caio Lima]

Created attachment 290488 [details]
Patch

This patch is rebasing with master and removing some debug code that I left in the last one.

As a reference for performance gains, I ran "super-get-by-id-with-this-monomorphic" with and without getter IC for op_get_by_id versions without merging the op_get_by_id_this. The result:

baseline                  changes                                      

super-get-by-id-with-this-monomorphic       52.2877+-0.9619     ^     31.3060+-0.8299        ^ definitely 1.6702x faster

<geometric>                                 52.2877+-0.9619     ^     31.3060+-0.8299        ^ definitely 1.6702x faster

Also I ran for benchmarks with --benchmarks "getter" and got ~30-40x faster, but I didn't figure out why this big difference yet. What I did to disable Getter IC was change https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/Repatch.cpp#L270 to ```return GivenUpOnCahe```.

The gains over ```super-get-by-id-with-this-monomorphic``` are explained because it uses self-getter to call the super-getter.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:77:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:79:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:140:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:68:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:111:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 30 files


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

[Filip Pizlo]

Why would we do this?

It's definitely good for lower tiers that get_by_id does not take an extra this operand.  It's also easier to parse.  I don't understand why we would remove this optimization.

[Caio Lima]

(In reply to comment #19)
> Why would we do this?

If I didn't miss any context *_with_this were created to solve the bug on (https://bugs.webkit.org/show_bug.cgi?id=147064). The current implementationis just a call to slow paths "operation*WithThis", meaning that we don't use IC or PIC on "super.foo" for example.
Merging get_by_id_with_this with op_get_by_id we enable its optimization including IC and PIC. In terms of performance, now it means 20% faster over op_get_by_id_with_this version.

> It's definitely good for lower tiers that get_by_id does not take an extra
> this operand.  It's also easier to parse.  I don't understand why we would
> remove this optimization.

I didn't understand why it is easier to parse.
Also, About good parts of get_by_id does not take an extra this operand, do you mind explain me why? I can think that we use extra memory to store the this operand, which has a big impact because get_by_id is one of the most used op_codes.

[Caio Lima]

Created attachment 291021 [details]
Patch for landing

Ok, now checking checking real gains for this patch. I created a new microbenchmarks test called ```super-getter.js``` and got results closer to expected:

super-getter                               183.8272+-3.5435     ^     33.8001+-0.9678        ^ definitely 5.4387x faster

Also, it seems neutral on other benchmarks.

[WebKit Commit Bot]

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


ERROR: JSTests/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:77:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:79:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:140:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:68:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:111:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 31 files


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

[Caio Lima]

Created attachment 291022 [details]
Patch

[Caio Lima]

Created attachment 291023 [details]
Benchmarks results

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:77:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:79:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:140:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:68:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:111:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 31 files


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

[Build Bot]

Comment on attachment 291022 [details]
Patch

Attachment 291022 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2245666

New failing tests:
js/regress-141098.html

[Build Bot]

Created attachment 291025 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

(In reply to comment #20)
> (In reply to comment #19)
> > Why would we do this?
> 
> If I didn't miss any context *_with_this were created to solve the bug on
> (https://bugs.webkit.org/show_bug.cgi?id=147064). The current
> implementationis just a call to slow paths "operation*WithThis", meaning
> that we don't use IC or PIC on "super.foo" for example.
> Merging get_by_id_with_this with op_get_by_id we enable its optimization
> including IC and PIC. In terms of performance, now it means 20% faster over
> op_get_by_id_with_this version.

Then you should make get_by_id_with_this use inline caches. 

You shouldn't change get_by_id. 

> 
> > It's definitely good for lower tiers that get_by_id does not take an extra
> > this operand.  It's also easier to parse.  I don't understand why we would
> > remove this optimization.
> 
> I didn't understand why it is easier to parse.
> Also, About good parts of get_by_id does not take an extra this operand, do
> you mind explain me why? I can think that we use extra memory to store the
> this operand, which has a big impact because get_by_id is one of the most
> used op_codes.

Compiler analyses always have an O(E) component, where E is the number of data flow edges. One such analysis is the OSR likeness analysis in the DFG and FTL. It's a very expensive analysis and we don't want to make it worse. You're adding a useless "this" data flow edge to every property access. I don't think we want that. 

I just want to be very clear: I'm strongly against uniting get_by_id with get_by_id_with_this. Please change your approach to making the unification happen at the PolymorphicAxcess/StructureStubInfo level and please keep these opcodes sepaate.

[Saam Barati]

I agree with Fil. I think we should just abstract the IC machinery to allow for get_by_id_with_this semantics. It probably requires modifying PolymorphicAccess, a new IC generator for the baseline, and proper handling in the DFG/FTL. I suspect you'll be able to abstract certain code in the DFG/FTL to handle both nodes. Also, I suspect the heart of the work you already did was in PolymorphicAccess and StructureStubInfo, so all of that code should not really be changed regardless of if the opcodes are united or not.

I'm renaming the bug. I guess I named the bug with the assumption that we should unite them, however, it would have been better if I had just said that get_by_id_with_this should use inline caching.

[Caio Lima]

(In reply to comment #28)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Why would we do this?
> > 
> > If I didn't miss any context *_with_this were created to solve the bug on
> > (https://bugs.webkit.org/show_bug.cgi?id=147064). The current
> > implementationis just a call to slow paths "operation*WithThis", meaning
> > that we don't use IC or PIC on "super.foo" for example.
> > Merging get_by_id_with_this with op_get_by_id we enable its optimization
> > including IC and PIC. In terms of performance, now it means 20% faster over
> > op_get_by_id_with_this version.
> 
> Then you should make get_by_id_with_this use inline caches. 
> 
> You shouldn't change get_by_id. 
> 
> > 
> > > It's definitely good for lower tiers that get_by_id does not take an extra
> > > this operand.  It's also easier to parse.  I don't understand why we would
> > > remove this optimization.
> > 
> > I didn't understand why it is easier to parse.
> > Also, About good parts of get_by_id does not take an extra this operand, do
> > you mind explain me why? I can think that we use extra memory to store the
> > this operand, which has a big impact because get_by_id is one of the most
> > used op_codes.
> 
> Compiler analyses always have an O(E) component, where E is the number of
> data flow edges. One such analysis is the OSR likeness analysis in the DFG
> and FTL. It's a very expensive analysis and we don't want to make it worse.
> You're adding a useless "this" data flow edge to every property access. I
> don't think we want that. 

Thank you for this explanation. I implemented a logic to avoid create an edge if ```base == this``` just thinking in memory usage, but it is interesting know that it also affects analyses performance.
 
> I just want to be very clear: I'm strongly against uniting get_by_id with
> get_by_id_with_this. Please change your approach to making the unification
> happen at the PolymorphicAxcess/StructureStubInfo level and please keep
> these opcodes sepaate.

Agreed. I don't like the idea of an operand for ```this``` in all property access on op_get_by_id since it is useless ~95% of time. 

(In reply to comment #29)
> I agree with Fil. I think we should just abstract the IC machinery to allow
> for get_by_id_with_this semantics. It probably requires modifying
> PolymorphicAccess, a new IC generator for the baseline, and proper handling
> in the DFG/FTL. I suspect you'll be able to abstract certain code in the
> DFG/FTL to handle both nodes. Also, I suspect the heart of the work you
> already did was in PolymorphicAccess and StructureStubInfo, so all of that
> code should not really be changed regardless of if the opcodes are united or
> not.

Cool. I am going to work on that.

[Caio Lima]

Created attachment 293705 [details]
Patch

[Caio Lima]

Created attachment 293718 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/Repatch.h:59:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 28 files


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

[Caio Lima]

(In reply to comment #32)
> Created attachment 293718 [details]
> Patch

It is almost done, but I am not happy with code replicated on cachedGetByIdWithThis. Anyway, I think it is in a good shape to receive feedback. Also, lets see what are robot results.

[Build Bot]

Comment on attachment 293718 [details]
Patch

Attachment 293718 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2453985

New failing tests:
js/dfg-double-vote-fuzz.html

[Build Bot]

Created attachment 293748 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 293804 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/Repatch.h:59:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 28 files


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

[Saam Barati]

Comment on attachment 293804 [details]
Patch

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

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:833
>      const Identifier& ident = *state.ident;

Have you tested this code when we have the result/thisGPR be aliased to each other? What about when an exception is thrown and they're aliased and you're in the DFG/FTL? You should prove to yourself that these work, and also write test cases that stress them (at least for now, since register allocation may change in the future)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4370
> +        base.use();
> +        thisValue.use();

This is not correct, we may need these if we throw an exception. So we're not yet done using these inputs.
(I know this is done in other places, and those places are also wrong).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4338
> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(baseGPR)));
> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisValueGPR)));

Can you add modes to this node where we have CellUse for child1 and child2? Seems like this will be the common case, and the DFG may have already proven that these are cells.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855
> +        m_out.branch(
> +            isCell(base, provenType(m_node->child1())), unsure(cellCase), unsure(notCellCase));

What about child2() being a cell? Also, see above comment about having a form where we have CellUse for these things.

> Source/JavaScriptCore/jit/Repatch.cpp:347
> +void repatchGetByID(ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PropertySlot& slot, StructureStubInfo& stubInfo, GetByIDKind kind, FunctionPtr repatchCallOperation)

I would add a new GetByIDKind and change the appropriateGenerecGetByIdFunction instead of changing this method to take an extra parameter.

[Saam Barati]

Comment on attachment 293804 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:15
> +        With inline cached enabled, ```super.member``` are ~4.5x faster,

Nice!

[Caio Lima]

Created attachment 296081 [details]
Patch Updated

This Patch is adding speculated paths when base and this are cell cases. Also it is fixing some comments made by Saamy and rebased with master to consider IC for DOMJIT.

Lets see what EWS think about it.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


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

[Caio Lima]

Comment on attachment 293804 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:833
>>      const Identifier& ident = *state.ident;
> 
> Have you tested this code when we have the result/thisGPR be aliased to each other? What about when an exception is thrown and they're aliased and you're in the DFG/FTL? You should prove to yourself that these work, and also write test cases that stress them (at least for now, since register allocation may change in the future)

I analyzed the code and noticed that it can cause problems when thisGPR and valueRegs are aliased and valueRegs is used during the IC code. valueRegs are used in the following cases:

1. When m_type is GetGetter or Load. m_type is one of these type if AccessKind == Pure, and for GetByIdWithThis, it is always AccessKind == WithThis.
2. When viaProxy is true. To satisfy this condition, base cell type on tryCacheGetByID need to be PureForwardingProxyType (in our case, JSGlobalObject). I created the following test case to reproduce the case:

```
class Base {
    get value() { return this._value; }
};

let o = { test() { return super.value; } };
this.value = o.test;

Object.setPrototypeOf(this, Base.prototype);

this._value = 3;

print(this.value()); 
```
However it results on: 
```
Exception: TypeError: Cannot set prototype of this object
setPrototypeOf@[native code]
global code@../js-tests/global-super.js:8:22
```

To see if any problem happens on our current tests suite, I changed the code to always alias valueRegs and thisGPR. No problem where found and the code ruined correctly on all tiers (tested on x86_64 only).
After this analysis, I couldn't think in test cases to stress it. Did you think in some cases?

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4370
>> +        thisValue.use();
> 
> This is not correct, we may need these if we throw an exception. So we're not yet done using these inputs.
> (I know this is done in other places, and those places are also wrong).

Reveted.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4338
>> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisValueGPR)));
> 
> Can you add modes to this node where we have CellUse for child1 and child2? Seems like this will be the common case, and the DFG may have already proven that these are cells.

Done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855
>> +            isCell(base, provenType(m_node->child1())), unsure(cellCase), unsure(notCellCase));
> 
> What about child2() being a cell? Also, see above comment about having a form where we have CellUse for these things.

Nice catch! Done.

>> Source/JavaScriptCore/jit/Repatch.cpp:347
>> +void repatchGetByID(ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PropertySlot& slot, StructureStubInfo& stubInfo, GetByIDKind kind, FunctionPtr repatchCallOperation)
> 
> I would add a new GetByIDKind and change the appropriateGenerecGetByIdFunction instead of changing this method to take an extra parameter.

You are right. It is much better readable.

[Caio Lima]

Created attachment 296082 [details]
Patch Updated

Fixing ARMv7 case for setupArgumentsWithExecState.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


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

[Caio Lima]

Created attachment 296093 [details]
Benchmark results

[Caio Lima]

Created attachment 296872 [details]
Patch

[Caio Lima]

Rebased the code with updates on master. Let's see what EWS thinks of that.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


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

[Saam Barati]

Comment on attachment 296872 [details]
Patch

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

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;

Is this corrects? Have you tested it?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:264
> +    if (spillMode == DontSpill) {

This should always be a constant for this function. Please remove and make it just baked into the code.

[Caio Lima]

Comment on attachment 296872 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
>> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;
> 
> Is this corrects? Have you tested it?

I didn't create a test for that, however I think it is necessary. I'm going to double check that.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:264
>> +    if (spillMode == DontSpill) {
> 
> This should always be a constant for this function. Please remove and make it just baked into the code.

ok

[Caio Lima]

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
>> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;
> 
> Is this corrects? Have you tested it?

I didn't create a test for that, however I think it is necessary. I'm going to double check that.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:264
>> +    if (spillMode == DontSpill) {
> 
> This should always be a constant for this function. Please remove and make it just baked into the code.

ok

[Radar WebKit Bug Importer]

<rdar://problem/29763183>

[Caio Lima]

Created attachment 297745 [details]
WIP Patch

Added case to test DOM JIT Inline Cache cases.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


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

[Caio Lima]

Created attachment 297746 [details]
Patch Updated

Submitted wrong diff. I think we are almost done now. Since it you are ok, I can write the ChangeLog properly.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


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

[Caio Lima]

ping review

[Saam Barati]

Comment on attachment 297746 [details]
Patch Updated

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

This patch looks pretty good. Here are some comments on various bugs I see.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;

Is this correct? Does DOMJIT expect the property holder or the receiver? This is a good question for Yusuke.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1154
> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;

Do you have tests for this?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2080
> +    JITCompiler::Call callOperation(J_JITOperation_ESsiJJI operation, JSValueRegs result, StructureStubInfo* stubInfo, GPRReg arg1Tag, GPRReg arg1Payload, GPRReg arg2Tag, GPRReg arg2Payload, UniquedStringImpl* uid)

These could be JSValueRegs.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:288
> +        slowPath = slowPathCall(
> +            slowCases, this, operationGetByIdWithThisOptimize,
> +            JSValueRegs(resultTagGPR, resultPayloadGPR), gen.stubInfo(), JSValueRegs(baseTagGPROrNone, basePayloadGPR), JSValueRegs(thisTagGPR, thisPayloadGPR), identifierUID(identifierNumber));

You should assert here that both tags are not invalid.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4368
> +            JSValueOperand base(this, node->child1());
> +            JSValueOperand thisValue(this, node->child2());

This is wrong. If either base or this here are CellUse, you're failing to speculate, which is incorrect. One way to solve this is to only allow to use kind states for this node:
1 && 2 are both CellUse
1 && 2 are both Untyped use.

You could ensure this inside the FixupPhase. Otherwise, you need to handle all 4 possibilities.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4355
> +            JSValueOperand base(this, node->child1());
> +            GPRReg baseGPR = base.gpr();
> +            JSValueOperand thisValue(this, node->child2());
> +            GPRReg thisValueGPR = thisValue.gpr();

Same comment here as in 32 bit implementation.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:695
> +    Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT0, regT1, ident->impl());

It seems like given your fast path code, these registers aren't guaranteed to be materialized to the values you want. Maybe load all values on the fast path before emitting a branch? Please add a test for this as it should probably be an insta-crash.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:704
> +    Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT1, regT0, regT4, regT3, ident->impl());

ditto here.

[Yusuke Suzuki]

Comment on attachment 297746 [details]
Patch Updated

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
>> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;
> 
> Is this correct? Does DOMJIT expect the property holder or the receiver? This is a good question for Yusuke.

DOMJIT custom getter assumes that the base value should be the DOM object.
So it is imcompatible with op_get_by_id_with_this.
For now, you can drop the DOMJIT acceleration support for `baseGPR != thisGPR` case.

[Yusuke Suzuki]

btw, this op_get_by_id_with_this is very nice.
I think we can construct the IC for numbers using this later... this = number, base = NumberPrototype.

[Caio Lima]

Comment on attachment 297746 [details]
Patch Updated

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

>>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
>>> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;
>> 
>> Is this correct? Does DOMJIT expect the property holder or the receiver? This is a good question for Yusuke.
> 
> DOMJIT custom getter assumes that the base value should be the DOM object.
> So it is imcompatible with op_get_by_id_with_this.
> For now, you can drop the DOMJIT acceleration support for `baseGPR != thisGPR` case.

Done.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1154
>> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
> 
> Do you have tests for this?

Yes. They are in JSTests/stress/super-get-by-id.js#91.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2080
>> +    JITCompiler::Call callOperation(J_JITOperation_ESsiJJI operation, JSValueRegs result, StructureStubInfo* stubInfo, GPRReg arg1Tag, GPRReg arg1Payload, GPRReg arg2Tag, GPRReg arg2Payload, UniquedStringImpl* uid)
> 
> These could be JSValueRegs.

Actually, this one isn't necessary. Just removing it. The version with JSValueRegs is in line 2070.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4368
>> +            JSValueOperand thisValue(this, node->child2());
> 
> This is wrong. If either base or this here are CellUse, you're failing to speculate, which is incorrect. One way to solve this is to only allow to use kind states for this node:
> 1 && 2 are both CellUse
> 1 && 2 are both Untyped use.
> 
> You could ensure this inside the FixupPhase. Otherwise, you need to handle all 4 possibilities.

Thanks for the catch. Done.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:695
>> +    Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT0, regT1, ident->impl());
> 
> It seems like given your fast path code, these registers aren't guaranteed to be materialized to the values you want. Maybe load all values on the fast path before emitting a branch? Please add a test for this as it should probably be an insta-crash.

Nice catch around that. I didn't find cases that can clobber regT0 and regT1 here, maybe you can help me think on that. Look the IC code generate for the following example:

```
let obj = {
   method() { return super.foo; }
};
```

Here is the IC Assembly:
Generated JIT code for Access stub for jaz#D4sAX5:[0x1053682e0->0x1053b6e40, BaselineFunctionCall, 36] bc#28 with return point CodePtr(0x3b99796019b6): Getter:(Generated, structure 
= 0x105374720:[Object, {foo:0}, NonArray, Proto:0x1053ac0a0, Leaf], offset = 0, callLinkInfo = 0x1047eb7f8):
    Code at [0x3b9979601e60, 0x3b9979601f00):
      0x3b9979601e60: cmp $0x108, (%rax)
      0x3b9979601e66: jnz 0x3b9979601b25
      0x3b9979601e6c: mov 0x10(%rax), %rdx
      0x3b9979601e70: mov $0x1c, 0x24(%rbp)
      0x3b9979601e77: mov 0x10(%rdx), %rdx
      0x3b9979601e7b: test %rdx, %rdx
      0x3b9979601e7e: jz 0x3b9979601ece
      0x3b9979601e84: sub $0x20, %rsp
      0x3b9979601e88: mov $0x1, 0x10(%rsp)
      0x3b9979601e90: mov %rdx, 0x8(%rsp)
      0x3b9979601e95: mov %rsi, 0x18(%rsp)
      0x3b9979601e9a: mov $0x0, %r11
      0x3b9979601ea4: cmp %r11, %rdx
      0x3b9979601ea7: jnz 0x3b9979601eb7
      0x3b9979601ead: call 0x3b9979601eb2
      0x3b9979601eb2: jmp 0x3b9979601ed8
      0x3b9979601eb7: mov %rdx, %rax
      0x3b9979601eba: mov $0x1047eb7f8, %rdx
      0x3b9979601ec4: call 0x3b9979601da0
      0x3b9979601ec9: jmp 0x3b9979601ed8
      0x3b9979601ece: mov $0xa, %rax
      0x3b9979601ed8: lea -0x50(%rbp), %rsp
      0x3b9979601edc: jmp 0x3b99796019b6
      0x3b9979601ee1: jmp 0x3b9979601b25
Generated JIT code for InlineAccess: linking constant jump:
    Code at [0x3b997960199f, 0x3b997960199f):
      0x3b997960199f: jmp 0x3b9979601e60

Operations that clobbers regT0 (in x86, it is %eax) are 0x3b9979601eb7 (slowPath when the function isn't compiled yet) and 0x3b9979601ece (return undefined case). Both cases doesn't go to slowPath_get_by_id_with_this.
Also, we can't emmit before the branch in JITGetByIDWithThis::generateFastPath, because the code from there is just a jump for slowPath and nops to reserve space to IC code in the first moment:

[  28] get_by_id_with_this loc8, loc7, this, foo(@id2)    predicting Stringident
          0x3b997960198e: mov -0x40(%rbp), %rax
          0x3b9979601992: mov 0x28(%rbp), %rsi
          0x3b9979601996: test %rax, %r15
          0x3b9979601999: jnz 0x3b9979601b25
          0x3b997960199f: jmp 0x3b9979601b25
          0x3b99796019a4: o16 nop %cs:0x200(%rax,%rax)
          0x3b99796019b3: nop (%rax)
          0x3b99796019b6: mov %rax, 0x1047873f0
          0x3b99796019c0: mov %rax, -0x48(%rbp)

The jump is changed when IC code is compiled and updated, so if we want to assure that the registers are materialized, I can add emit emitGetVirtualRegister() before callOperation in JIT::emitSlow_op_get_by_id_with_this.
It is important to notice that JIT::emitSlow_op_get_by_id is also trusting that its reg is materialized, but in that case, it is just regT0. If we find a bug for op_get_by_id_with_this, probably it will also be reproducible in op_get_by_id.
I'm going to add 2 test cases to reproduce if the registers can be clobbered and break stuff in the next Patch.
.

[Caio Lima]

Created attachment 300054 [details]
Patch With Bug Fixes

It is fixing almost all Saamy's comments. I've analyzed the case of register materialization and couldn't find a reproducible bug. Anyway, I added JSTests/stress/super-force-ic-fail.js to test paths handled by IC when getter is changed or deleted.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


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

[Caio Lima]

Created attachment 300066 [details]
Patch rebased

Patch rebased. Hope it compiles now.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


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

[Yusuke Suzuki]

Comment on attachment 300066 [details]
Patch rebased

I cannot find the part preventing us from using DOMJIT for op_get_by_id_with_this. Could you point this part?

[Caio Lima]

(In reply to comment #67)
> Comment on attachment 300066 [details]
> Patch rebased
> 
> I cannot find the part preventing us from using DOMJIT for
> op_get_by_id_with_this. Could you point this part?

I'm not sure if I understood. The previous Patch was configuring a "this" operand of a DOMJIT IC however I removed that code. Should we also cancel emmit IC in that case?

Here is the code in the old Patch:

https://bugs.webkit.org/attachment.cgi?id=297746&action=diff#a/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp_sec2

[Yusuke Suzuki]

(In reply to comment #68)
> (In reply to comment #67)
> > Comment on attachment 300066 [details]
> > Patch rebased
> > 
> > I cannot find the part preventing us from using DOMJIT for
> > op_get_by_id_with_this. Could you point this part?
> 
> I'm not sure if I understood. The previous Patch was configuring a "this"
> operand of a DOMJIT IC however I removed that code. Should we also cancel
> emmit IC in that case?
> 
> Here is the code in the old Patch:
> 
> https://bugs.webkit.org/attachment.cgi?id=297746&action=diff#a/Source/
> JavaScriptCore/bytecode/PolymorphicAccess.cpp_sec2

Yes, DOMJIT IC should not be emitted for op_get_by_id_with_this case since it is imcompatible.

[Caio Lima]

(In reply to comment #69)
> Yes, DOMJIT IC should not be emitted for op_get_by_id_with_this case since
> it is imcompatible.

Cool. Sorry for don't understand it before. I'm going to update that.

[Caio Lima]

Created attachment 300268 [details]
Patch Updated

This patch is considering don't emmit IC for get_by_id_with_this when it is a DOMJIT.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


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

[Saam Barati]

Comment on attachment 300268 [details]
Patch Updated

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

> JSTests/stress/super-get-by-id.js:116
> +for (let i = 0; i < 1000000; i++) {
> +    try {
> +        getterValue(subObj);
> +        assert(false);
> +    } catch(e) {
> +        assert(e instanceof TypeError);
> +    };
> +}

Please add some more interesting exception tests where the exception handler has to recover values.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1153
> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;

I think the above comment makes this code wrong. We should have tests for this. Do you already have tests?
According to the above comment:
- CustomValue should be passed the property holder, which might not necessarily be |this|, however, your code says that it is.
- Custom accessors are indeed passed |this|.

Please add tests for both of these.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:261
> +    if (basePayloadGPR == resultTagGPR) {
> +        RELEASE_ASSERT(basePayloadGPR != resultPayloadGPR);
> +        
> +        if (baseTagGPROrNone == resultPayloadGPR) {
> +            m_jit.swap(basePayloadGPR, baseTagGPROrNone);
> +            baseTagGPROrNone = resultTagGPR;
> +        } else
> +            m_jit.move(basePayloadGPR, resultPayloadGPR);
> +        
> +        basePayloadGPR = resultPayloadGPR;
> +    }
> +    
> +    // Repeat the same logic for this payload and result tag.
> +    if (thisPayloadGPR == resultTagGPR) {
> +        RELEASE_ASSERT(thisPayloadGPR != resultPayloadGPR);
> +        
> +        if (thisTagGPR == resultPayloadGPR) {
> +            m_jit.swap(thisPayloadGPR, thisTagGPR);
> +            thisTagGPR = resultTagGPR;
> +        } else
> +            m_jit.move(thisPayloadGPR, resultPayloadGPR);
> +        
> +        thisPayloadGPR = resultPayloadGPR;
> +    }

This is only possible in `cachedGetById` because it uses the `Reuse` tag when allocating the result register. You don't do that here, so it should be impossible for this to ever be true.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4391
> +            base.use();
> +            thisValue.use();
> +            

I know GetById does this, but it's slightly wrong. You're saying you're done with this the resulting values of these nodes at this point, but it's not quite true.  If an exception is thrown, you'll need their values.

(The fact that GetById calls use() is something I want to fix at some point, it's somewhat arguable that any node should be doing this manually now that the DFG handles exceptions)

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869
> +        } else if (m_node->child1().useKind() == CellUse || m_node->child2().useKind() == CellUse) {

This case should now be impossible, please remove.

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:123
> +    UniquedStringImpl*, JSValueRegs base, JSValueRegs value, JSValueRegs thisRegs, AccessType accessType)

Style nit:
Your register ordering is a bit off with how we usually do it. The result should either be the first or the last register. It's against the style we usually have that it's in between to operands.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:676
> +    emitLoad(base, regT1, regT0);
> +    emitJumpSlowCaseIfNotJSCell(base, regT1);
> +    emitLoad(thisVReg, regT4, regT3);
> +    emitJumpSlowCaseIfNotJSCell(thisVReg, regT4);

This still has the problem I was talking about in my review of an earlier patch. The slow case does not handle regT3/regT4 not being populated.

> Source/JavaScriptCore/jit/Repatch.cpp:299
> +            // we don't emmit IC for DOMJIT when op is get_by_id_with_this

typo: "emmit" => "emit"

[Caio Lima]

Comment on attachment 300268 [details]
Patch Updated

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1153
>> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
> 
> I think the above comment makes this code wrong. We should have tests for this. Do you already have tests?
> According to the above comment:
> - CustomValue should be passed the property holder, which might not necessarily be |this|, however, your code says that it is.
> - Custom accessors are indeed passed |this|.
> 
> Please add tests for both of these.

I'm confused now. When we have a customAccessor, thisValue is the receiver, in that case it shouldn't be the "thisGPR"? I created a test to cover the customAccessorGetter:

```
// CustomGetter case
 
 let customGetter = createCustomGetterObject();
 Object.setPrototypeOf(customGetter, Object.prototype);
 
 let subObj = {
     __proto__: customGetter,
     get value () {
         return super.customGetterAccessor;
     }
 }
 
 for (let i = 0; i < 1000000; i++) {
     let value = getterValue(subObj);
     assert(value == 100);
 }

 subObj.shouldThrow = true;
 for (let i = 0; i < 1000000; i++) {
     try {
         getterValue(subObj);
         assert(false);
     } catch(e) {
         assert(e instanceof TypeError);
     };
 }
```

If I don't set baseForCustomValue with thisGPR, this test case will fail. Is that correct behavior?

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869
>> +        } else if (m_node->child1().useKind() == CellUse || m_node->child2().useKind() == CellUse) {
> 
> This case should now be impossible, please remove.

Nice catch. My bad.

>> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:676
>> +    emitJumpSlowCaseIfNotJSCell(thisVReg, regT4);
> 
> This still has the problem I was talking about in my review of an earlier patch. The slow case does not handle regT3/regT4 not being populated.

My bad =(. Just forgot do modify it before.

[Caio Lima]

Created attachment 300775 [details]
Patch Updated

Now this patch contains tests for CustomValue and Custom accessor and also a test case with exception handler.

I've also renamed PolymorphicAccess.cpp variable baseForCustomValue to baseForCustom, since it actually is used as the base for CustomValue and CustomAccessor.

Let's see what EWS thinks about it.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 30 files


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

[Caio Lima]

Created attachment 302034 [details]
Patch

[Caio Lima]

(In reply to comment #77)
> Created attachment 302034 [details]
> Patch

Rebased with master

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 31 files


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

[Saam Barati]

Comment on attachment 302034 [details]
Patch

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

r=me

> Source/JavaScriptCore/jit/Repatch.cpp:153
> +inline FunctionPtr appropriateGenericGetByIdFunction(GetByIDKind kind)
>  {
>      if (kind == GetByIDKind::Normal)
>          return operationGetById;
> +    else if (kind == GetByIDKind::WithThis)
> +        return operationGetByIdWithThisGeneric;
>      return operationTryGetById;
>  }

you also need to fill this out for appropriateOptimizingGetByIdFunction. That should probably lead to an insta crash if we ever got the wrong thing. At least, it should lead to noticeably not correct behavior. Please add a test for this.

[Caio Lima]

Created attachment 302467 [details]
Patch for landing

This Patch is addressing last Saamy's comment about appropriateOptimizingGetByIdFunction. It is trigged when the StructureStubClearingWatchpoint is fired and property is not watchable anymore. The test case in JSTests/stress/super-getter-reset-ic.js is validating this path.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 33 files


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

[Saam Barati]

Comment on attachment 302467 [details]
Patch for landing

r=me

[Yusuke Suzuki]

Caio, if you would like to land it as is, please set "cq=?".
I (or, maybe other JSC reviewers) can set it to "cq=+". Then the patch will be landed through the commit queue. :)

[Caio Lima]

(In reply to comment #84)
> Caio, if you would like to land it as is, please set "cq=?".
> I (or, maybe other JSC reviewers) can set it to "cq=+". Then the patch will
> be landed through the commit queue. :)

Done =)

[WebKit Commit Bot]

Comment on attachment 302467 [details]
Patch for landing

Clearing flags on attachment: 302467

Committed r213019: <http://trac.webkit.org/changeset/213019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The JSC test for this change is timing out on debug:

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20JSC%20%28Tests%29/builds/437

There also appear to be 73 tests asserting during 32-bit JSC testing: 

stress/super-get-by-id.js.default: ASSERTION FAILED: !info.alive()
stress/super-get-by-id.js.default: /Volumes/Data/slave/sierra-32bitJSC-debug/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp(1773) : void JSC::DFG::SpeculativeJIT::compileCurrentBlock()
stress/super-get-by-id.js.default: 1   0x11f283a WTFCrash
stress/super-get-by-id.js.default: 2   0x902c27 JSC::DFG::SpeculativeJIT::compileCurrentBlock()
stress/super-get-by-id.js.default: 3   0x90383f JSC::DFG::SpeculativeJIT::compile()
stress/super-get-by-id.js.default: 4   0x79828c JSC::DFG::JITCompiler::compileBody()
stress/super-get-by-id.js.default: 5   0x79d70a JSC::DFG::JITCompiler::compileFunction()
stress/super-get-by-id.js.default: 6   0x8a6593 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
stress/super-get-by-id.js.default: 7   0x8a3593 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
stress/super-get-by-id.js.default: 8   0x70ca99 JSC::DFG::compileImpl(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue> const&, WTF::Ref<JSC::DeferredCompilationCallback>&&)
stress/super-get-by-id.js.default: 9   0x70c499 JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue> const&, WTF::Ref<JSC::DeferredCompilationCallback>&&)
stress/super-get-by-id.js.default: 10  0xc4ce06 operationOptimize
stress/super-get-by-id.js.default: 11  0x334cc1c
stress/super-get-by-id.js.default: 12  0xe3fd2c vmEntryToJavaScript
stress/super-get-by-id.js.default: 13  0xc2c378 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
stress/super-get-by-id.js.default: 14  0xbd51cf JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
stress/super-get-by-id.js.default: 15  0x414857 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
stress/super-get-by-id.js.default: 16  0xa9ede8 JSC::callGetter(JSC::ExecState*, JSC::JSValue, JSC::JSValue)
stress/super-get-by-id.js.default: 17  0x103671d JSC::PropertySlot::functionGetter(JSC::ExecState*) const
stress/super-get-by-id.js.default: 18  0x2ebf0e JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const
stress/super-get-by-id.js.default: 19  0x2ebcc4 JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
stress/super-get-by-id.js.default: 20  0xc43968 operationGetById
stress/super-get-by-id.js.default: 21  0x3214f77
stress/super-get-by-id.js.default: 22  0x320b48c
stress/super-get-by-id.js.default: 23  0xe3fd2c vmEntryToJavaScript
stress/super-get-by-id.js.default: 24  0xc2c378 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
stress/super-get-by-id.js.default: 25  0xbd440d JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
stress/super-get-by-id.js.default: 26  0x4ef1a5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
stress/super-get-by-id.js.default: 27  0x9bb31 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool, bool, bool)
stress/super-get-by-id.js.default: 28  0x592a7 jscmain(int, char**)::$_7::operator()(JSC::VM&, GlobalObject*) const
stress/super-get-by-id.js.default: 29  0x499b0 int runJSC<jscmain(int, char**)::$_7>(CommandLine, jscmain(int, char**)::$_7 const&)
stress/super-get-by-id.js.default: 30  0x4837b jscmain(int, char**)
stress/super-get-by-id.js.default: 31  0x482e7 main
stress/super-get-by-id.js.default: test_script_7560: line 2: 43316 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useFTLJIT\=true super-get-by-id.js )
stress/super-get-by-id.js.default: ERROR: Unexpected exit code: 139

https://build.webkit.org/builders/Apple%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/720

[Saam Barati]

Ima roll it out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 168925

[Caio Lima]

These regressions are due the absence of ```base.use()``` and ```thisValue.use()``` in DFGSpeculative32_64.cpp. The assertion that is failing is https://github.com/caiolima/webkit/blob/get-by-id-this-ic/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp#L1770

How could I fix that? Remove the assertion, adding ```.use()``` or other approach?

[Saam Barati]

Comment on attachment 302467 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4346
> +            jsValueResult(resultTagGPR, resultPayloadGPR, node, UseChildrenCalledExplicitly);

The assertion is because you're passing UseChildfenCalledExplicitly here. Because you're not calling use(), you should not pass this value. I think the default parameter will not pass this value.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4366
> +            jsValueResult(resultTagGPR, resultPayloadGPR, node, UseChildrenCalledExplicitly);

Ditto

[Caio Lima]

Created attachment 303454 [details]
Patch for landing

This Patch is fixing regressions on x86_32 and --debug timeout tests.

Thank you for the directions Saamy =).

[Caio Lima]

Created attachment 303455 [details]
Patch for landing rebased

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:124:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:116:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 33 files


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

[WebKit Commit Bot]

Comment on attachment 303455 [details]
Patch for landing rebased

Clearing flags on attachment: 303455

Committed r213467: <http://trac.webkit.org/changeset/213467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.