[webkit-reviews] review granted: [Bug 185695] DFG should inline InstanceOf ICs : [Attachment 340759] the patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat May 19 12:38:04 PDT 2018
Yusuke Suzuki <utatane.tea at gmail.com> has granted Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 185695: DFG should inline InstanceOf ICs
https://bugs.webkit.org/show_bug.cgi?id=185695
Attachment 340759: the patch
https://bugs.webkit.org/attachment.cgi?id=340759&action=review
--- Comment #15 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 340759
--> https://bugs.webkit.org/attachment.cgi?id=340759
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.
More information about the webkit-reviews
mailing list