Summary: | DFG should inline InstanceOf ICs | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2018-05-16 13:48:58 PDT
Created attachment 340714 [details]
it begins
Created attachment 340724 [details]
a bit more
Created attachment 340731 [details]
maye it is written
Created attachment 340740 [details]
it did things
Created attachment 340748 [details]
it's a nice speed-up
Created attachment 340751 [details]
the patch
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.
Created attachment 340755 [details]
the patch
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.
Created attachment 340756 [details]
the patch
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.
Created attachment 340759 [details]
the patch
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 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. (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 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 :) |