[RuntimeAsync] Fix support for pinvokes

[jakobbotsch]

• Make sure async transformation leaves a scratch BB around for the pinvoke prolog to be inserted into
• Make sure we properly insert pinvoke epilogs for GT_RETURN_SUSPEND nodes (needed on x86)
• Ensure the pinvoke frame related locals are not captured by the async transformation

[jakobbotsch]

PTAL @VSadov

[VSadov]

A thought: what should happen if UnmanagedCallersOnly is applied to runtime async method.
Does it work today with ordinary async? (I guess, why not, they are just Task-returning methods)

[jakobbotsch]

I haven't checked, but I imagine UnmanagedCallersOnly on any function returning an object reference should result in IlegalProgramException. So I would hope that was the case for async1, and we should similarly disallow it for async2.

[VSadov]

I haven't checked, but I imagine UnmanagedCallersOnly on any function returning an object reference should result in IlegalProgramException. So I would hope that was the case for async1, and we should similarly disallow it for async2.

I checked.
Even though the documentation only mentions that parameters must be blittable, the runtime requires that the return type is blittable as well. That rules out runtime async signatures. Even nongeneric ValueType, although a struct, is not blittable.

[VSadov]

Do you plan to address this TODO or this is just a "good to have" comment?

[VSadov]

I do agree that relying on the presence of scratch is an arbitrary dependency that just happen to hold.
If creating on demand is not too hard, it would be better.

[jakobbotsch]

Do you plan to address this TODO or this is just a "good to have" comment?

I don't plan to address it here, this should rather be cleaned up upstream.

I do agree that relying on the presence of scratch is an arbitrary dependency that just happen to hold.
If creating on demand is not too hard, it would be better.

The frontend does ensure it:

runtimelab/src/coreclr/jit/flowgraph.cpp

Lines 2315 to 2322 in 4daa761

	// The backend requires a scratch BB into which it can safely insert a P/Invoke method prolog if one is
	// required. Similarly, we need a scratch BB for poisoning and when we have Swift parameters to reassemble.
	// Create it here.
	if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame() || lvaHasAnySwiftStackParamToReassemble())
	{
	madeChanges |= fgEnsureFirstBBisScratch();
	fgFirstBB->SetFlags(BBF_DONT_REMOVE);
	}

It's simply that this is a very long-term invariant that is error prone and which feels unnecessary to me.

[VSadov]

Exactly. Such dependencies have negative value by default. They must be justified by advantages or scenarios that they enable. If arranging a scratch block was difficult to do later, it could be a possible justification, but, as I understand, it is not the case.

[VSadov]

LGTM. Thanks!