Bug 185695

Summary: DFG should inline InstanceOf ICs
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 185652    
Bug Blocks: 185784, 185804, 185803    
Attachments:
Description Flags
it begins
none
a bit more
none
maye it is written
none
it did things
none
it's a nice speed-up
none
the patch
none
the patch
none
the patch
none
the patch ysuzuki: review+

Description Filip Pizlo 2018-05-16 13:48:58 PDT
...
Comment 1 Filip Pizlo 2018-05-18 11:09:24 PDT
Created attachment 340714 [details]
it begins
Comment 2 Filip Pizlo 2018-05-18 12:04:24 PDT
Created attachment 340724 [details]
a bit more
Comment 3 Radar WebKit Bug Importer 2018-05-18 12:13:14 PDT
<rdar://problem/40373500>
Comment 4 Filip Pizlo 2018-05-18 13:09:18 PDT
Created attachment 340731 [details]
maye it is written
Comment 5 Filip Pizlo 2018-05-18 14:50:13 PDT
Created attachment 340740 [details]
it did things
Comment 6 Filip Pizlo 2018-05-18 15:30:04 PDT
Created attachment 340748 [details]
it's a nice speed-up
Comment 7 Filip Pizlo 2018-05-18 15:55:35 PDT
Created attachment 340751 [details]
the patch
Comment 8 EWS Watchlist 2018-05-18 15:57:56 PDT
Attachment 340751 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Filip Pizlo 2018-05-18 16:15:01 PDT
Created attachment 340755 [details]
the patch
Comment 10 EWS Watchlist 2018-05-18 16:18:47 PDT
Attachment 340755 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Filip Pizlo 2018-05-18 16:23:53 PDT
Created attachment 340756 [details]
the patch
Comment 12 EWS Watchlist 2018-05-18 16:26:15 PDT
Attachment 340756 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Filip Pizlo 2018-05-18 16:41:24 PDT
Created attachment 340759 [details]
the patch
Comment 14 EWS Watchlist 2018-05-18 16:44:33 PDT
Attachment 340759 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Yusuke Suzuki 2018-05-19 12:38:04 PDT
Comment on attachment 340759 [details]
the patch

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

r=me with nits.

> Source/JavaScriptCore/bytecode/InstanceOfVariant.h:37
> +    InstanceOfVariant() { }

Use `InstanceOfVariant() = default;`

> Source/JavaScriptCore/bytecode/InstanceOfVariant.h:61
> +    JSObject* m_prototype = nullptr;

Make this `JSObject* m_prototype { nullptr }`.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3389
> +        if (base.m_structure.isInfinite() || baseSet.size() < base.m_structure.size())

Is `baseSet.size() < base.m_structure.size()` condition correct? If node->matchStructureData().variants have so many structures, which includes all the base.m_structure, baseSet.size() == base.m_structure.size(), but we have a chance to reduce # of variants.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3390
> +            m_state.setFoundConstants(true);

I think we have a chance to clear NodeMustGenerate in constant folding phase if the given abstract value's structure set is finite and all the structures are covered by MatchStructure's set.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:520
> +                    RegisteredStructureSet structureSet;
> +                    for (MatchStructureVariant& variant : data.variants)
> +                        structureSet.add(variant.structure);
> +                    addBaseCheck(indexInBlock, node, baseValue, structureSet);
> +                    m_graph.convertToConstant(
> +                        node, m_graph.freeze(jsBoolean(result == BooleanLattice::True)));

`changed = true`?

> Source/WTF/wtf/BooleanLattice.h:42
> +inline BooleanLattice lubBooleanLattice(BooleanLattice a, BooleanLattice b)
> +{
> +    return static_cast<BooleanLattice>(static_cast<uint8_t>(a) | static_cast<uintptr_t>(b));
> +}

I think `leastUpperBoundOfBooleanLattices` is better since we already have bunch of `leastUpperBoundXXX` functions.
Comment 16 Filip Pizlo 2018-05-19 12:47:34 PDT
(In reply to Yusuke Suzuki from comment #15)
> Comment on attachment 340759 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340759&action=review
> 
> r=me with nits.
> 
> > Source/JavaScriptCore/bytecode/InstanceOfVariant.h:37
> > +    InstanceOfVariant() { }
> 
> Use `InstanceOfVariant() = default;`

Fixed.  Can you tell me what the advantage of this is?

> 
> > Source/JavaScriptCore/bytecode/InstanceOfVariant.h:61
> > +    JSObject* m_prototype = nullptr;
> 
> Make this `JSObject* m_prototype { nullptr }`.

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3389
> > +        if (base.m_structure.isInfinite() || baseSet.size() < base.m_structure.size())
> 
> Is `baseSet.size() < base.m_structure.size()` condition correct? If
> node->matchStructureData().variants have so many structures, which includes
> all the base.m_structure, baseSet.size() == base.m_structure.size(), but we
> have a chance to reduce # of variants.

You're right.  This should be baseSet.size() < node->matchStructureData().variants.size().  I fixed it and will retest perf.

> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3390
> > +            m_state.setFoundConstants(true);
> 
> I think we have a chance to clear NodeMustGenerate in constant folding phase
> if the given abstract value's structure set is finite and all the structures
> are covered by MatchStructure's set.

We don't usually clear NodeMustGenerate in other cases where this happens, so I didn't do it here.  In FTL mode, it really doesn't matter, since we rely on B3 to do the real DCE.

> 
> > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:520
> > +                    RegisteredStructureSet structureSet;
> > +                    for (MatchStructureVariant& variant : data.variants)
> > +                        structureSet.add(variant.structure);
> > +                    addBaseCheck(indexInBlock, node, baseValue, structureSet);
> > +                    m_graph.convertToConstant(
> > +                        node, m_graph.freeze(jsBoolean(result == BooleanLattice::True)));
> 
> `changed = true`?

You're right!  Fixed.

> 
> > Source/WTF/wtf/BooleanLattice.h:42
> > +inline BooleanLattice lubBooleanLattice(BooleanLattice a, BooleanLattice b)
> > +{
> > +    return static_cast<BooleanLattice>(static_cast<uint8_t>(a) | static_cast<uintptr_t>(b));
> > +}
> 
> I think `leastUpperBoundOfBooleanLattices` is better since we already have
> bunch of `leastUpperBoundXXX` functions.

Right.  Fixed.
Comment 17 Yusuke Suzuki 2018-05-19 12:55:23 PDT
Comment on attachment 340759 [details]
the patch

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

>>> Source/JavaScriptCore/bytecode/InstanceOfVariant.h:37
>>> +    InstanceOfVariant() { }
>> 
>> Use `InstanceOfVariant() = default;`
> 
> Fixed.  Can you tell me what the advantage of this is?

The compiler can offer a good information without adding an explicit annotation.
For example, if all the members are constexpr-initialized, constructor becomes `constexpr` constructor :)
Comment 18 Filip Pizlo 2018-05-19 15:01:30 PDT
Landed in https://trac.webkit.org/changeset/232000/webkit