[webkit-reviews] review denied: [Bug 214861] [JSC] support op_get_private_name in DFG and FTL : [Attachment 410149] Patch

[bugzilla-daemon at webkit.org]

Caio Lima <ticaiolima at gmail.com> has denied Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 214861: [JSC] support op_get_private_name in DFG and FTL
https://bugs.webkit.org/show_bug.cgi?id=214861

Attachment 410149: Patch

https://bugs.webkit.org/attachment.cgi?id=410149&action=review




--- Comment #19 from Caio Lima <ticaiolima at gmail.com> ---
Comment on attachment 410149
  --> https://bugs.webkit.org/attachment.cgi?id=410149
Patch

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

I'm giving r- because I've found a bug for 32-bits (bots are also red).
Overall, patch LGTM besides 32-bits issues.

> Source/JavaScriptCore/bytecode/GetByStatus.cpp:111
> +    case op_get_private_name:

I think this is something we'd like to add in future patches. DO you mind add a
comment or even a FIXME to use IC information from LLInt to populate
GetByStatus?

Also, can we double check that GetByIdStatus is doing the right thing for
`get_private_name`? We have stronger requirements than common `get_by_[val|id]`
because we need to throw exception when private field is not present in an
object and never lookup prototype chain. Since we do Unset and Prototype load
IC, it would be good to include ASSERTs on paths we don't expect to see
`get_private_name` going.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1786
> +		   fixEdge<SymbolUse>(node->child2());

I think we should blindly speculate symbol here when it is `GetPrivateName`,
like we are doing for PutPrivateName. I don't think we are expecting to see any
other type here.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1789
> +		   fixEdge<CellUse>(node->child1());

Is there a reason we want to have UntypedUse for `base`? I don't think we have
profitable paths there. We are always fixing base to cell for both
`PutPrivateName` and `GetPrivateName`. This also would simplify a lot
SpeculativeJIT code.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
> +    JSValueOperand base(this, m_graph.child(node, 0),
ManualOperandSpeculation);

We are missing a Cell speculation here if `node->child1().useKind() ==
CellUse`. But if we decide to always speculate Cell for base (as proposed on
Fixup) we cna simply use `SpeculateCellOperand`. Doing so helps on register
pressure for 32-bits architectures.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3505
> +	   resultRegs, TrustedImmPtr::weakPointer(m_graph,
m_graph.globalObjectFor(codeOrigin)), gen.stubInfo(), baseRegs, propertyRegs);

I'm not 100% sure if this is 32-bits friendly, since we only have payloadOnly
for propertyRegs. We need to include CCallHelpers::CellValue there.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3531
> +    case UntypedUse: {

If we decide to always speculate cell on base, we don't need this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3947
> +	   LValue property = lowJSValue(node->child2(),
ManualOperandSpeculation);

We can make base always Cell and property always symbol here.