New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DFG should support tuples #11086
DFG should support tuples #11086
Conversation
EWS run on previous version of this PR (hash 340db27) |
340db27
to
7e7c552
Compare
EWS run on previous version of this PR (hash 7e7c552) |
7e7c552
to
bfc0ba5
Compare
EWS run on previous version of this PR (hash bfc0ba5) |
bfc0ba5
to
5edde02
Compare
EWS run on previous version of this PR (hash 5edde02) |
flushRegisters(); | ||
JSValueRegsFlushedCallResult result(this); | ||
JSValueRegs resultRegs = result.regs(); | ||
// FIXME: We should teach callOperation how to handle more than one result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we support two result GPRs. See extractResult etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extractResult doesn't quite work because it does a single register on 64-bit. I guess I could do:
setupArguments<decltype(operationEnumeratorNextUpdateIndexAndMode)>(LinkableConstant::globalObject(*this, node), baseRegs, indexGPR, modeGPR, enumeratorGPR);
appendCallSetResult(operationEnumeratorNextUpdateIndexAndMode, resultGPR1, resultGPR2);
setPrediction(SpecInt32Only); | ||
case ExtractFromTuple: { | ||
// Use mergePrediction because ExtractFromTuple doesn't know if the prediction could change. | ||
mergePrediction(m_tupleSpeculations[m_currentNode->tupleIndex()]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about having this prediction in TupleData? This can be useful in FixupPhase too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you do that here? This is the prediction on the extract node.
struct TupleData { | ||
uint16_t refCount { 0 }; | ||
uint16_t resultFlags { 0 }; | ||
VirtualRegister virtualRegister; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can extend this further, like having m_prediction etc. Basically, this can be a data structure containing most of DFG::Node
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't really need the prediction here because consumers of the node will see the prediction via the ExtractFromTuple. I could see adding OpInfos here for extra metadata as needed but it seems like we should do that when we need it.
unsigned tupleOffset() const | ||
{ | ||
ASSERT(isTuple()); | ||
return m_virtualRegister.toLocal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit unfortunate, but yeah, it works since we never allocate VirtualRegister for tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figured saving the OpInfos for other data seemed more useful since we know that we're not going to use the virtualRegister for anything.
ASSERT(m_graph.m_tupleData.at(node->tupleIndex()).virtualRegister == node->virtualRegister()); | ||
VirtualRegister virtualRegister = node->virtualRegister(); | ||
GenerationInfo& info = generationInfoFromVirtualRegister(virtualRegister); | ||
info.stealForNode(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we should have a bit better name for that, like, initFromTupleElement
?
And don't we need to setup this GenerationInfo based on resultFlags of tuple element? (As the same way to FTL)
Maybe, the producer side already initialize this GenerationInfo with appropriate types, and since we are using the same VirtualRegister here, we do not need to initialize it much, but maybe we should assert here to ensure that the resulted GenerationInfo matches against the expected Tuple element's type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's right I was relying on the fact that we are just repurposing the layout of the tuple's result. Sure, I can add those ASSERTs.
5edde02
to
e00e56f
Compare
EWS run on previous version of this PR (hash e00e56f) |
e00e56f
to
96e7b43
Compare
EWS run on previous version of this PR (hash 96e7b43) |
96e7b43
to
3aadb9a
Compare
EWS run on previous version of this PR (hash 3aadb9a) |
3aadb9a
to
224d40e
Compare
EWS run on previous version of this PR (hash 224d40e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
@@ -662,6 +662,12 @@ class Validater { | |||
VALIDATE(!value->kind().hasExtraBits(), ("At ", *value)); | |||
VALIDATE(value->numChildren() >= 1, ("At ", *value)); | |||
VALIDATE(value->child(0)->type() == pointerType(), ("At ", *value)); | |||
if (value->type().isTuple()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add comment that currently we are only supporting two-element case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (!inst.args[0].isSpecial()) | ||
return false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this check twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to delete it here. Will delete.
for (unsigned i = inst.args.size() - 1; i > 1; --i) { | ||
Arg arg = inst.args[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not visit args[1]
. i
starts with size - 1, and we run this loop if i > 1
, this means i needs to be 2 or more. So, it misses the visit to callee
, is there any reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, it should include callee. I think I confused myself with the args layout for CCallSpecial when we lower to that.
|
||
jsValueResult(resultRegs, node); | ||
useChildren(node); | ||
strictInt32TupleResultWithoutUsingChildren(newIndex.gpr(), node, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to check refCount
for this first tuple element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't bother constant folding it because it's the index. If we did you'd need to check yeah but you'd trigger an assert to notice you did it wrong.
return; | ||
} | ||
|
||
if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && baseEdge.useKind() == CellUse) { | ||
SpeculateCellOperand base(this, baseEdge); | ||
JSValueRegsTemporary result(this); | ||
GPRTemporary newIndex(this); | ||
GPRTemporary newMode(this, Reuse, mode); | ||
GPRReg baseGPR = base.gpr(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's first declare all GPRs here like,
GPRReg newIndexGPR = newIndex.gpr();
GPRReg newModeGPR = newMode.gpr();
since gpr() is side-effectful operation. If we jump over that, it causes the bug. So it is always nice if we get all GPRs first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it used to be side-effectful but .*Temporary.gpr()
is not anymore, which IMO causes a bit of an anti pattern where the operands you declared can get spilled before they are filled. .*Operand
s still get filled on first use so those have to be initialized early.
return; | ||
} | ||
|
||
JSValueOperand base(this, baseEdge); | ||
#if USE(JSVALUE64) | ||
GPRTemporary newMode(this, Reuse, mode); | ||
#endif | ||
JSValueRegs baseRegs = base.regs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Let's get all GPRs here first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
strictInt32TupleResultWithoutUsingChildren(indexResult.gpr(), node, 0); | ||
strictInt32TupleResultWithoutUsingChildren(modeResult.gpr(), node, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to check refCount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll just change it so we check in strictInt32TupleResultWithoutUsingChildren
.
ASSERT(m_graph.m_tupleData.at(node->tupleIndex()).virtualRegister == node->virtualRegister()); | ||
VirtualRegister virtualRegister = node->virtualRegister(); | ||
GenerationInfo& info = generationInfoFromVirtualRegister(virtualRegister); | ||
if (ASSERT_ENABLED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, this causes some build failures. Let's just use #if ASSERT_ENABLED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Code& code = special->code(); | ||
|
||
size_t resultCount = cCallResultCount(code, value); | ||
size_t expectedArgCount = resultCount + 1; // first Arg is always CCallSpecial. | ||
for (Value* child : value->children()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it is including callee itself.
for (size_t n = cCallResultCount(code, value); n; --n) { | ||
Type type = value->type().isTuple() ? code.proc().typeAtOffset(value->type(), n - 1) : value->type(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n
is decremented. But next
's index
is incremented. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it doesn't matter right now because if there's more than one result you get the same registers anyway. In the future it could matter though so I'll fix.
224d40e
to
1ae27f9
Compare
EWS run on previous version of this PR (hash 1ae27f9) |
1ae27f9
to
500b65f
Compare
500b65f
to
4e4244e
Compare
EWS run on previous version of this PR (hash 4e4244e)
|
4e4244e
to
0303753
Compare
EWS run on previous version of this PR (hash 0303753)
|
0303753
to
ab3884c
Compare
EWS run on current version of this PR (hash ab3884c)
|
https://bugs.webkit.org/show_bug.cgi?id=253413 Reviewed by Yusuke Suzuki. This change adds support for tuples in the DFG. It works similarly to how tuples work in B3 where there is a side buffer which holds most of the metadata for a tuple. In the DFG there are three pieces of information held here: 1) The reference count 2) The result flags 3) The virtual register 1) tells us if the ExtractFromTuple Node for a given tuple result still exists (e.g. it could have been constant folded or dead code eliminated). From that we will decide to fill (3) the virtual register when allocating registers. If we didn't have the reference count then we would have no way to know that the virtual registers isn't going to be consumed thus we will leak it. Lastly we have (2) the result flags for the node. This tells the ExtractFromTuple and theoretically (as right now the only ExtractFromTuple users produce Int32s) any consumers of the ExtractFromTuple what value format the tuple's result will be in. For the DFG, we don't need this because we know there's at most one ExtractFromTuple for any given tuple index since we don't duplicate code. When dumping the DFG graph ExtractFromTuple follows the same pattern as B3 and uses `<<X` to denote the offset we are extracting from. So it will look something like: 6 1 0: D@101:< 1:-> EnumeratorNextUpdateIndexAndMode(Check:Untyped:D@97, Check:Untyped:D@98, Check:Untyped:D@100, Check:Untyped:D@99, VarArgs, SelectUsingPredictions+NonArray+InBounds+AsIs+Read, enumeratorModes = 4, R:World, W:Heap, Exits, ClobbersExit, bc#115, ExitValid) 7 1 0: D@102:< 1:-> ExtractFromTuple(Check:Untyped:D@101, Int32|UseAsOther, <<0, bc#115, ExitInvalid) 8 1 0: D@103:<!0:-> MovHint(Check:Untyped:D@102, MustGen, loc10, W:SideState, ClobbersExit, bc#115, ExitInvalid) 9 1 0: D@104:< 1:-> ExtractFromTuple(Check:Untyped:D@101, Int32|UseAsOther, <<1, bc#115, ExitInvalid) This patch also adds support for calling operations in both the FTL/B3 via CCall. CCall can take exactly the tuple of `{ pointerType(), pointerType() }`, which, for every calling conevention we support, should be returned in both the return value registers. As the only way to look into a tuple is via the B3 prodecure, the first Air::Arg of any CCall/ColdCCall Inst is now the CCallSpecial for the compiling Air::Code. This gives us access to Air::Code inside CCallCustom::forEachArg and isValidForm. * JSTests/stress/for-in-redefine-enumerable.js: (shouldBe): * Source/JavaScriptCore/b3/B3LowerToAir.cpp: * Source/JavaScriptCore/b3/B3Type.h: (JSC::B3::pointerType): (JSC::B3::registerType): * Source/JavaScriptCore/b3/B3Validate.cpp: * Source/JavaScriptCore/b3/air/AirCCallingConvention.cpp: (JSC::B3::Air::cCallResultCount): (JSC::B3::Air::cCallArgumentRegisterWidth): (JSC::B3::Air::cCallResult): * Source/JavaScriptCore/b3/air/AirCCallingConvention.h: * Source/JavaScriptCore/b3/air/AirCustom.cpp: (JSC::B3::Air::CCallCustom::isValidForm): * Source/JavaScriptCore/b3/air/AirCustom.h: (JSC::B3::Air::CCallCustom::forEachArg): * Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp: (JSC::B3::Air::lowerAfterRegAlloc): * Source/JavaScriptCore/b3/air/AirLowerMacros.cpp: (JSC::B3::Air::lowerMacros): * Source/JavaScriptCore/b3/air/AirOpcode.opcodes: * Source/JavaScriptCore/b3/testb3.h: * Source/JavaScriptCore/b3/testb3_3.cpp: (addCallTests): * Source/JavaScriptCore/b3/testb3_5.cpp: (JSC_DEFINE_JIT_OPERATION): (testCallPairResult): (testCallPairResultRare): * Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h: (JSC::DFG::AbstractInterpreter::setTupleConstant): * Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h: (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): * Source/JavaScriptCore/dfg/DFGAtTailAbstractState.cpp: (JSC::DFG::AtTailAbstractState::AtTailAbstractState): * Source/JavaScriptCore/dfg/DFGAtTailAbstractState.h: (JSC::DFG::AtTailAbstractState::forTupleNode): (JSC::DFG::AtTailAbstractState::clearForTupleNode): (JSC::DFG::AtTailAbstractState::setForTupleNode): (JSC::DFG::AtTailAbstractState::setTypeForTupleNode): (JSC::DFG::AtTailAbstractState::setNonCellTypeForTupleNode): (JSC::DFG::AtTailAbstractState::makeBytecodeTopForTupleNode): (JSC::DFG::AtTailAbstractState::makeHeapTopForTupleNode): * Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: (JSC::DFG::ByteCodeParser::addToGraph): (JSC::DFG::ByteCodeParser::parseBlock): * Source/JavaScriptCore/dfg/DFGClobberize.h: (JSC::DFG::clobberize): * Source/JavaScriptCore/dfg/DFGDoesGC.cpp: (JSC::DFG::doesGC): * Source/JavaScriptCore/dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::fixupNode): * Source/JavaScriptCore/dfg/DFGGenerationInfo.h: (JSC::DFG::GenerationInfo::initFromTupleResult): * Source/JavaScriptCore/dfg/DFGGraph.cpp: (JSC::DFG::Graph::dump): * Source/JavaScriptCore/dfg/DFGGraph.h: * Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp: (JSC::DFG::InPlaceAbstractState::InPlaceAbstractState): * Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h: (JSC::DFG::InPlaceAbstractState::forTupleNode): (JSC::DFG::InPlaceAbstractState::clearForTupleNode): (JSC::DFG::InPlaceAbstractState::setForTupleNode): (JSC::DFG::InPlaceAbstractState::setTypeForTupleNode): (JSC::DFG::InPlaceAbstractState::setNonCellTypeForTupleNode): (JSC::DFG::InPlaceAbstractState::makeBytecodeTopForTupleNode): (JSC::DFG::InPlaceAbstractState::makeHeapTopForTupleNode): * Source/JavaScriptCore/dfg/DFGMayExit.cpp: * Source/JavaScriptCore/dfg/DFGNode.h: (JSC::DFG::Node::isTuple const): (JSC::DFG::Node::setTupleOffset): (JSC::DFG::Node::tupleOffset const): (JSC::DFG::Node::hasExtractOffset const): (JSC::DFG::Node::extractOffset const): (JSC::DFG::Node::tupleIndex const): (JSC::DFG::Node::tupleSize const): (JSC::DFG::Node::hasVirtualRegister): (JSC::DFG::Node::virtualRegister): (JSC::DFG::Node::setVirtualRegister): * Source/JavaScriptCore/dfg/DFGNodeType.h: * Source/JavaScriptCore/dfg/DFGOperations.cpp: (JSC::DFG::JSC_DEFINE_JIT_OPERATION): * Source/JavaScriptCore/dfg/DFGOperations.h: (JSC::DFG::makeUGPRPair): * Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp: * Source/JavaScriptCore/dfg/DFGSafeToExecute.h: (JSC::DFG::safeToExecute): * Source/JavaScriptCore/dfg/DFGScoreBoard.h: (JSC::DFG::ScoreBoard::use): * Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp: * Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h: (JSC::DFG::SpeculativeJIT::strictInt32TupleResultWithoutUsingChildren): * Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp: (JSC::DFG::SpeculativeJIT::compile): * Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp: (JSC::DFG::SpeculativeJIT::compile): * Source/JavaScriptCore/dfg/DFGValidate.cpp: * Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp: (JSC::DFG::VirtualRegisterAllocationPhase::run): * Source/JavaScriptCore/ftl/FTLCapabilities.cpp: (JSC::FTL::canCompile): * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp: (JSC::FTL::DFG::LowerDFGToB3::LowerDFGToB3): (JSC::FTL::DFG::LowerDFGToB3::compileNode): (JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq): * Source/JavaScriptCore/ftl/FTLOutput.cpp: (JSC::FTL::Output::extract): * Source/JavaScriptCore/ftl/FTLOutput.h: * Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h: (JSC::Wasm::AirIRGeneratorBase::emitCCall): Canonical link: https://commits.webkit.org/262068@main
ab3884c
to
f2f3c91
Compare
Committed 262068@main (f2f3c91): https://commits.webkit.org/262068@main Reviewed commits have been landed. Closing PR #11086 and removing active labels. |
f2f3c91
ab3884c
π§ͺ api-iosπ§ͺ mac-AS-debug-wk2