JIT: Avoid implicit byref retbufs in async calls #115888
Conversation
The check in the importer did not take into account that a `LCL_ADDR` of an implicit byref local will turn into `LCL_VAR` after morph. The async transformation requires the retbuf to be the address of a local, so enhance the check to take this into account. Also switch the `compIsAsync()` check to `call->IsAsync()`. Inside async functions it is ok for non-async calls to use the normal logic.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @EgorBo |
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes return-buffer legality checks by introducing impIsLegalRetBuf and replaces ad-hoc logic in impStoreStruct with calls to the new helper, ensuring proper handling for async and non-async calls.
- Replaces duplicated inline checks in
impStoreStructwithimpIsLegalRetBuf. - Implements
Compiler::impIsLegalRetBufto encapsulate ABI and async invariants. - Adds the new helper’s declaration to
compiler.h.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/importer.cpp | Replaced inline retbuf checks with impIsLegalRetBuf; implemented helper |
| src/coreclr/jit/compiler.h | Declared impIsLegalRetBuf |
Comments suppressed due to low confidence (1)
src/coreclr/jit/importer.cpp:1081
- Consider adding unit or integration tests for
impIsLegalRetBuf, especially covering async calls with implicit byref locals and non-async scenarios with byref-like return types.
bool Compiler::impIsLegalRetBuf(GenTree* retBuf, GenTreeCall* call)
| @@ -1068,6 +1062,46 @@ GenTree* Compiler::impStoreStruct(GenTree* store, | |||
| return store; | |||
| } | |||
|
|
|||
| //------------------------------------------------------------------------ | |||
| // impIsLegalRetbuf: | |||
There was a problem hiding this comment.
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.
| // impIsLegalRetbuf: | |
| // impIsLegalRetBuf: |
Copilot uses AI. Check for mistakes.
| @@ -4797,6 +4797,7 @@ class Compiler | |||
| Statement** pAfterStmt = nullptr, | |||
| const DebugInfo& di = DebugInfo(), | |||
| BasicBlock* block = nullptr); | |||
There was a problem hiding this comment.
[nitpick] Add a brief Doxygen-style comment above this declaration to explain when impIsLegalRetBuf should be used and its ABI/async guarantees.
| 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.
The check in the importer did not take into account that a
LCL_ADDRof an implicit byref local will turn intoLCL_VARafter morph. The async transformation requires the retbuf to be the address of a local, so enhance the check to take this into account.Also switch the
compIsAsync()check tocall->IsAsync(). Inside async functions it is ok for non-async calls to use the normal logic.Fix #115884