Bug 185695 - DFG should inline InstanceOf ICs
Summary: DFG should inline InstanceOf ICs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on: 185652
Blocks: 185784 185804 185803
  Show dependency treegraph
 
Reported: 2018-05-16 13:48 PDT by Filip Pizlo
Modified: 2018-05-19 15:01 PDT (History)
5 users (show)

See Also:


Attachments
it begins (18.87 KB, patch)
2018-05-18 11:09 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (31.52 KB, patch)
2018-05-18 12:04 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maye it is written (39.67 KB, patch)
2018-05-18 13:09 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it did things (55.60 KB, patch)
2018-05-18 14:50 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it's a nice speed-up (63.20 KB, patch)
2018-05-18 15:30 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (63.35 KB, patch)
2018-05-18 15:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (63.41 KB, patch)
2018-05-18 16:15 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (63.41 KB, patch)
2018-05-18 16:23 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (63.98 KB, patch)
2018-05-18 16:41 PDT, Filip Pizlo
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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