JIT: Avoid implicit byref retbufs in async calls

[jakobbotsch]

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.

Fix #115884

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @EgorBo

[Copilot]

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 impStoreStruct with impIsLegalRetBuf.
• Implements Compiler::impIsLegalRetBuf to 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)

[Copilot]

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]

[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.
	*/

[EgorBo]

So #115884 happened because retbuf arg was spilled to a local?

[jakobbotsch]

So #115884 happened because retbuf arg was spilled to a local?

We basically have source code like

void Foo(Vector128<byte> v)
{
  v = M();
}

We ended up with LCL_ADDR v as the retbuf after import because the code in the importer allows it through since it is LCL_ADDR, but then morph changes that to LCL_VAR v since v is an implicit byref. And the async transformation expects to see LCL_ADDR.

[jakobbotsch]

/ba-g Azurelinux problems