Skip to content

JIT: Avoid implicit byref retbufs in async calls #115888

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+43 −8
Open

JIT: Avoid implicit byref retbufs in async calls #115888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e1a37b
JIT: Avoid implicit byref retbufs in async calls
jakobbotsch May 22, 2025
f42c489
Remove comment
jakobbotsch May 22, 2025
3e96fea
Nit
jakobbotsch May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4797,6 +4797,7 @@ class Compiler
Statement** pAfterStmt = nullptr,
const DebugInfo& di = DebugInfo(),
BasicBlock* block = nullptr);
Copy link
Preview

Copilot AI May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Add a brief Doxygen-style comment above this declaration to explain when impIsLegalRetBuf should be used and its ABI/async guarantees.

Suggested change
BasicBlock* block = nullptr);
BasicBlock* block = nullptr);
/**
* @brief Determines if the specified return buffer is legal for the given call.
*
* This function checks whether the provided return buffer (`retBuf`) is valid
* for use with the specified call (`call`). It ensures compliance with the
* platform's ABI (Application Binary Interface) requirements and handles
* any constraints related to asynchronous operations.
*
* @param retBuf The return buffer to validate.
* @param call The call for which the return buffer is being validated.
* @return true if the return buffer is legal; false otherwise.
*/

Copilot uses AI. Check for mistakes.

bool impIsLegalRetBuf(GenTree* retBuf, GenTreeCall* call);
GenTree* impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned curLevel, GenTreeFlags indirFlags = GTF_EMPTY);

GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags);
Expand Down
50 changes: 42 additions & 8 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,10 +844,7 @@ GenTree* Compiler::impStoreStruct(GenTree* store,
GenTreeFlags indirFlags = GTF_EMPTY;
GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags);

// Make sure we don't pass something other than a local address to the return buffer arg.
// It is allowed to pass current's method return buffer as it is a local too.
if ((fgAddrCouldBeHeap(destAddr) && !eeIsByrefLike(srcCall->gtRetClsHnd)) ||
(compIsAsync() && !destAddr->OperIs(GT_LCL_ADDR)))
if (!impIsLegalRetBuf(destAddr, srcCall))
{
unsigned tmp = lvaGrabTemp(false DEBUGARG("stack copy for value returned via return buffer"));
lvaSetStruct(tmp, srcCall->gtRetClsHnd, false);
Expand Down Expand Up @@ -971,10 +968,7 @@ GenTree* Compiler::impStoreStruct(GenTree* store,
GenTreeFlags indirFlags = GTF_EMPTY;
GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags);

// Make sure we don't pass something other than a local address to the return buffer arg.
// It is allowed to pass current's method return buffer as it is a local too.
if ((fgAddrCouldBeHeap(destAddr) && !eeIsByrefLike(call->gtRetClsHnd)) ||
(compIsAsync() && !destAddr->OperIs(GT_LCL_ADDR)))
if (!impIsLegalRetBuf(destAddr, call))
{
unsigned tmp = lvaGrabTemp(false DEBUGARG("stack copy for value returned via return buffer"));
lvaSetStruct(tmp, call->gtRetClsHnd, false);
Expand Down Expand Up @@ -1068,6 +1062,46 @@ GenTree* Compiler::impStoreStruct(GenTree* store,
return store;
}

//------------------------------------------------------------------------
// impIsLegalRetbuf:
Copy link
Preview

Copilot AI May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment header uses impIsLegalRetbuf (lowercase 'b') but the function is named impIsLegalRetBuf. Update the comment to match the function’s exact casing for clarity.

Suggested change
// impIsLegalRetbuf:
// impIsLegalRetBuf:

Copilot uses AI. Check for mistakes.

// Check if a return buffer is of a legal shape.
//
// Arguments:
// retBuf - The return buffer
// call - The call that is passed the return buffer
//
// Return Value:
// True if it is legal according to ABI and IR invariants.
//
// Notes:
// ABI requires all return buffers to point to stack. Also, we have an IR
// invariant for async calls that return buffers must be the address of a
// local.
//
bool Compiler::impIsLegalRetBuf(GenTree* retBuf, GenTreeCall* call)
{
if (call->IsAsync())
{
// Async calls require LCL_ADDR shape for the retbuf to know where to
// save the value on resumption.
if (!retBuf->OperIs(GT_LCL_ADDR))
{
return false;
}

// LCL_ADDR on an implicit byref will turn into LCL_VAR in morph.
if (lvaIsImplicitByRefLocal(retBuf->AsLclVarCommon()->GetLclNum()))
{
return false;
}

return true;
}

// The ABI requires the retbuffer to point to stack.
return !fgAddrCouldBeHeap(retBuf) || eeIsByrefLike(call->gtRetClsHnd);
}

//------------------------------------------------------------------------
// impStoreStructPtr: Store (copy) the structure from 'src' to 'destAddr'.
//
Expand Down
Loading