Uh oh!
There was an error while loading. Please reload this page.
JIT: Add GT_SWIFT_ERROR_RET to represent loading error register upon return - #100692
Conversation
I'm not sure it's quite that simple due transforms within the JIT that assume knowledge about |
amanasifkhalid
commented
Apr 5, 2024
I see. In that case, let me try substituting |
jakobbotsch
commented
Apr 5, 2024
FWIW I think it's very likely we'll end up similarly having to expand the JIT's set of known and handled terminator nodes as part of async2. It has a similar situation with an async continuation that is returned in a different register than usual. I'm pretty sure the prototype has similar issues around return merging as the one I described above. |
amanasifkhalid
commented
Apr 5, 2024
This might be too radical, but what if we expanded |
jakobbotsch
commented
Apr 5, 2024
I'm not sure whether it makes things much simpler -- you still have to audit the interesting uses of A more uniform representation would be to introduce the ability for the JIT to create new class layouts on the fly. In fact we can already do that for layouts without GC pointers, which would be sufficient for Swift (see |
I think the uniform representation above would also generalize to It's probably a lot of work, but seems like it would be the right long term direction. |
amanasifkhalid
commented
Apr 5, 2024
I have a binop implementation of |
amanasifkhalid
commented
Apr 8, 2024
@jakobbotsch PTAL. |
jakobbotsch
commented
Apr 8, 2024
I'll take a closer look tomorrow, but I'm curious why the null op1 didn't work out given that we have some prior art with Might be a bit cleaner to introduce something like GenTree* GetReturnedValue(GenTreeOp* op)
{
assert(op->OperIs(GT_RETURN, GT_RETFILT, GT_SWIFT_ERROR_RET));
return op->OperIs(GT_SWIFT_ERROR_RET) ? op->gtGetOp2() : op->gtGetOp1();
}Mutating |
amanasifkhalid
commented
Apr 8, 2024
I initially tried that, but when handling simple operators in a bunch of places, we either assume binops have a first operand, or we assume the lack of a first operand implies the second operand is null as well (like in |
amanasifkhalid
commented
Apr 8, 2024
/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| GTNODE(SWIFT_ERROR , GenTree ,0,0,GTK_LEAF) // Error register value post-Swift call | ||
| GTNODE(SWIFT_ERROR , GenTree ,0,0,GTK_LEAF) // Error register value post-Swift call | ||
| GTNODE(SWIFT_ERROR_RET , GenTreeOp ,0,1,GTK_BINOP|GTK_NOVALUE) // Returns normal return value, and SwiftError pseudolocal's value in REG_SWIFT_ERROR |
There was a problem hiding this comment.
Would be nice with a comment that op1 has the error and op2 has the normal return value (or null if method returns void)
| // If this method has a SwiftError* out parameter, we need to set the error register upon returning. | ||
| // By placing the GT_SWIFT_ERROR_RET node before the GT_RETURN node, | ||
| // we don't have to teach the JIT that GT_SWIFT_ERROR_RET is a valid terminator for BBJ_RETURN blocks. | ||
| // During lowering, we will move the GT_SWIFT_ERROR_RET to the end of the block | ||
| // so the error register won't get trashed when returning the normal value. | ||
| if (lvaSwiftErrorArg != BAD_VAR_NUM) | ||
| { | ||
| assert(info.compCallConv == CorInfoCallConvExtension::Swift); | ||
| assert(lvaSwiftErrorLocal != BAD_VAR_NUM); | ||
| GenTree* const swiftErrorNode = gtNewLclFldNode(lvaSwiftErrorLocal, TYP_I_IMPL, 0); | ||
| op1 = new (this, GT_SWIFT_ERROR_RET) | ||
| GenTreeOp(GT_SWIFT_ERROR_RET, op1->TypeGet(), swiftErrorNode, op1->gtGetOp1()); | ||
| } |
There was a problem hiding this comment.
The comment is outdated now, right?
There was a problem hiding this comment.
You can use op1->SetOper(GT_SWIFT_ERROR_RET) instead here to avoid the new node allocation. Of course that requires some shuffling of the operands, so just allocating a new node is also ok with me, if you like that better feel free to ignore this.
| GenTree* Lowering::LowerSwiftErrorRet(GenTree* swiftErrorRet) | ||
| { | ||
| assert(swiftErrorRet->OperIs(GT_SWIFT_ERROR_RET)); | ||
| GenTree* const nextNode = swiftErrorRet->gtNext; | ||
| LIR::Range& blockRange = BlockRange(); | ||
| blockRange.Remove(swiftErrorRet); | ||
| blockRange.InsertAtEnd(swiftErrorRet); | ||
| return nextNode; | ||
| } |
There was a problem hiding this comment.
I assume this isn't needed anymore?
| for (BasicBlock* block : Blocks()) | ||
| { | ||
| if (block->KindIs(BBJ_RETURN)) | ||
| { | ||
| GenTree* const ret = block->lastNode(); | ||
| assert(ret->OperIs(GT_RETURN)); | ||
| ret->SetOperRaw(GT_SWIFT_ERROR_RET); | ||
| ret->AsOp()->gtOp2 = ret->AsOp()->gtOp1; | ||
| // If this is the merged return block, use the merged return error local as the error operand. | ||
| // Else, load the error value from the SwiftError pseudolocal (this will probably get promoted, anyway). | ||
| if (block == genReturnBB) | ||
| { | ||
| assert(genReturnErrorLocal == BAD_VAR_NUM); | ||
| genReturnErrorLocal = lvaGrabTemp(true DEBUGARG("Single return block SwiftError value")); | ||
| lvaGetDesc(genReturnErrorLocal)->lvType = genActualType(TYP_I_IMPL); | ||
| ret->AsOp()->gtOp1 = gtNewLclvNode(genReturnErrorLocal, TYP_I_IMPL); | ||
| } | ||
| else | ||
| { | ||
| ret->AsOp()->gtOp1 = gtNewLclFldNode(lvaSwiftErrorLocal, TYP_I_IMPL, 0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I still think it would be nice to do the replacement of lvaSwiftErrorArg with lvaSwiftErrorLocal here too.
There was a problem hiding this comment.
That was my plan, though I was hitting asserts around fgNodeThreading being invalid when trying to use any of the GenTree iterators -- LocalsGenTreeList seems like it would be the most efficient one for finding the SwiftError* arg uses. Is there a clean way of doing the IR traversal without breaking any of these iterators' invariants?
There was a problem hiding this comment.
At this point we don't have threaded IR nodes (the comment on fgNodeThreading has some relevant information), but you can use GenTreeVisitor to do it.
There was a problem hiding this comment.
optReplaceScalarUsesWithConst looks close to what you want to do I think.
There was a problem hiding this comment.
optReplaceScalarUsesWithConst looks close to what you want to do I think.
Thanks! GenTreeVisitor works.
| } | ||
| case GT_RETURN: | ||
| case GT_SWIFT_ERROR_RET: |
There was a problem hiding this comment.
I think if-conversion supports GT_RETURN to transform
JTRUE(EQ(x, 0)) -> BB01, BB02
BB01:
RETURN(0)
BB02:
RETURN(1)
into
RETURN(SELECT(EQ(x, 0), 0, 1))
I don't see us being able to support this transform for GT_SWIFT_ERROR_RET, so I think we can't add this support here.
| const DebugInfo& di = lastStmt->GetDebugInfo(); | ||
| GenTree* swiftErrorStore = | ||
| gtNewTempStore(genReturnErrorLocal, ret->gtGetOp1(), CHECK_SPILL_NONE, nullptr, di, block); |
There was a problem hiding this comment.
| const DebugInfo& di = lastStmt->GetDebugInfo(); | |
| GenTree* swiftErrorStore = | |
| gtNewTempStore(genReturnErrorLocal, ret->gtGetOp1(), CHECK_SPILL_NONE, nullptr, di, block); | |
| GenTree* swiftErrorStore = | |
| gtNewTempStore(genReturnErrorLocal, ret->gtGetOp1()); |
(For pAfterStmt == nullptr these last parameters aren't relevant)
| case GT_SWIFT_ERROR_RET: | ||
| if (tree->gtGetOp2() != nullptr) | ||
| { | ||
| tree->gtVNPair = vnStore->VNPWithExc(vnStore->VNPForVoid(), | ||
| vnStore->VNPExceptionSet(tree->gtGetOp2()->gtVNPair)); | ||
| } | ||
| else | ||
| { | ||
| tree->gtVNPair = vnStore->VNPForVoid(); | ||
| } | ||
| break; | ||
There was a problem hiding this comment.
This should have the exceptions from the error operand propagated too.
| fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) | ||
| { | ||
| if ((*use)->OperIs(GT_LCL_VAR) && ((*use)->AsLclVarCommon()->GetLclNum() == m_compiler->lvaSwiftErrorArg)) | ||
| { | ||
| *use = m_compiler->gtNewLclVarAddrNode(m_compiler->lvaSwiftErrorLocal, genActualType(*use)); | ||
| } | ||
| return fgWalkResult::WALK_CONTINUE; |
There was a problem hiding this comment.
Should we BADCODE here if we see a use of the error parameter that isn't a GT_LCL_VAR? E.g. any store or taking the address of it.
There was a problem hiding this comment.
That seems reasonable to me. Do you know if we have any plans upstream to regulate the usage of the error parameter? I imagine we wouldn't want the JIT to do these checks alone.
There was a problem hiding this comment.
I'm not sure. cc @jkoritzinsky@kotlarmilos. Does it seem appropriate to you to put restrictions on what the IL can do with the SwiftError* parameter? Specifically we're interested in disallowing stores and disallowing taking its address.
| { | ||
| if (!(*use)->OperIs(GT_LCL_VAR)) | ||
| { | ||
| BADCODE("Expected GT_LCL_VAR of SwiftError* parameter, got other GenTreeLclVarCommon node"); |
There was a problem hiding this comment.
I think the message should be phrased in terms of IL, not IR constructs
There was a problem hiding this comment.
Would something like "Found invalid use of SwiftError* parameter" be too vague?
amanasifkhalid
commented
Apr 11, 2024
/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| { | ||
| assert(genReturnErrorLocal == BAD_VAR_NUM); | ||
| genReturnErrorLocal = lvaGrabTemp(true DEBUGARG("Single return block SwiftError value")); | ||
| lvaGetDesc(genReturnErrorLocal)->lvType = genActualType(TYP_I_IMPL); |
There was a problem hiding this comment.
| lvaGetDesc(genReturnErrorLocal)->lvType = genActualType(TYP_I_IMPL); | |
| lvaGetDesc(genReturnErrorLocal)->lvType = TYP_I_IMPL; |
| #ifdef SWIFT_SUPPORT | ||
| if (m_comp->lvaSwiftErrorArg != BAD_VAR_NUM) | ||
| { | ||
| m_mainOper = GT_SWIFT_ERROR_RET; | ||
| } | ||
| else | ||
| #endif // SWIFT_SUPPORT | ||
| { | ||
| m_mainOper = GT_RETURN; | ||
| } |
There was a problem hiding this comment.
Are these changes necessary?
It looks like IfConvertCheckStmts will fail naturally for GT_SWIFT_ERROR_RET, so perhaps no changes in if-conversion are necessary?
There was a problem hiding this comment.
You're right, we don't check m_mainOper until after we call IfConvertCheckStmts, and we bail if the terminator node is a GT_SWIFT_ERROR_RET, so it should be safe to undo these changes.
| case GT_SWIFT_ERROR_RET: | ||
| LowerNode(node->gtGetOp1()); | ||
| FALLTHROUGH; | ||
| case GT_RETURN: |
There was a problem hiding this comment.
| caseGT_SWIFT_ERROR_RET: | |
| LowerNode(node->gtGetOp1()); | |
| FALLTHROUGH; | |
| caseGT_RETURN: | |
| caseGT_SWIFT_ERROR_RET: | |
| caseGT_RETURN: |
Otherwise we invoke lowering of op1 twice.
There was a problem hiding this comment.
LowerRet should probably be changed to take GenTreeOp* instead.
| tree->SetAllEffectsFlags(tree->AsOp()->gtGetOp1()); | ||
| tree->SetAllEffectsFlags(retVal); | ||
| return tree; |
There was a problem hiding this comment.
This case needs to invoke morph on op1 as well for GT_SWIFT_ERROR_RET (I am not sure why the code returns here in the first place, it probably ought to just fall through after inserting the cast)
Uh oh!
There was an error while loading. Please reload this page.
jakobbotsch
left a comment
There was a problem hiding this comment.
This looks great to me now. Thanks for your diligence in implementing this!
amanasifkhalid
commented
Apr 12, 2024
Thank you for all the reviews! In case you're curious, here's the updated codegen for Quite the improvement. |
…return (dotnet#100692) Follow-up to dotnet#100429. If a method has a `SwiftError*` out parameter, a new phase -- `fgAddSwiftErrorReturns` -- converts all `GT_RETURN` nodes into `GT_SWIFT_ERROR_RET` nodes; this new node type is a binop that takes the error value as its first operand, and the normal return value (if there is one) as its second operand. The error value is loaded into the Swift error register upon returning.
Follow-up to #100429. When creating return nodes during importation, if the method has a
SwiftError*out parameter, the JIT will create aGT_SWIFT_ERROR_RETnode before the regularGT_RETURNnode to represent loading theSwiftErrorpseudolocal's value into the error register. We initially keep this node before theGT_RETURNblock so we don't have to teach the JIT that it is a new valid terminator forBBJ_RETURNblocks; during lowering, we move this node to the end of the block so the error register won't get trashed after loading the error value. I found this to be an easier approach than trying to replaceGT_RETURNwith a binary node that holds the regular and error return values.Old codegen:
New codegen: