JIT: Avoid check for returns in methods with pinvokes in LSRA

[jakobbotsch]

The pinvoke epilog is explicit on all our targets now, so it seems like this code should be unnecessary everywhere (and in any case, should not have been inside very general and very hot newRefPosition).

[dotnet-policy-service]

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

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr jitstressregs

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

Failures are #103577, #103568, #103545, #102370, #102335, #102706

cc @dotnet/jit-contrib PTAL @kunalspathak

A few minor diffs, some good TP improvements.

[kunalspathak]

The pinvoke epilog is explicit on all our targets now

Can you point to the relevant code? When I search for RBM_PINVOKE_TCB, other than lsrabuild.cpp, I just see it getting used for arm/x86.

[jakobbotsch]

Can you point to the relevant code? When I search for RBM_PINVOKE_TCB, other than lsrabuild.cpp, I just see it getting used for arm/x86.

The pinvoke epilog is inserted as regular IR in lowering:

runtime/src/coreclr/jit/lower.cpp

Lines 5683 to 5695 in f2bdb75

	//------------------------------------------------------------------------
	// InsertPInvokeMethodEpilog: Code that needs to be run when exiting any method
	// that has PInvoke inlines. This needs to be inserted any place you can exit the
	// function: returns, tailcalls and jmps.
	//
	// Arguments:
	// returnBB - basic block from which a method can return
	// lastExpr - GenTree of the last top level stmnt of returnBB (debug only arg)
	//
	// Return Value:
	// Code tree to perform the action.
	//
	void Lowering::InsertPInvokeMethodEpilog(BasicBlock* returnBB DEBUGARG(GenTree* lastExpr))

I suspect the code I am removing here exists because at some point the pinvoke epilog was implicitly generated as part of epilog codegen (or perhaps GT_RETURN codegen). But since it is explicitly represented in IR today LSRA should already be handling the conflicts that may arise by normal means.

[kunalspathak]

Does that mean we do not need RBM_PINVOKE_FRAME and need RBM_PINVOKE_TCB only for x86/arm?

[jakobbotsch]

Does that mean we do not need RBM_PINVOKE_FRAME and need RBM_PINVOKE_TCB only for x86/arm?

REG_PINVOKE_FRAME is still used. It doesn't look like RBM_PINVOKE_FRAME has uses, but I guess it's nice to be consistent about making the mask versions available.

Both are only used in x86/arm32, so the definitions outside could be removed. Do you want me to do that as part of this PR?

[kunalspathak]

REG_PINVOKE_FRAME is still used. It doesn't look like RBM_PINVOKE_FRAME has uses, but I guess it's nice to be consistent about making the mask versions available.

The only use I see for REG_PINVOKE_FRAME is this, which is for x86/arm, so we can just keep it for x86/arm and remove it from other targets.

runtime/src/coreclr/jit/gentree.cpp

Lines 1780 to 1783 in 35437dd

	#if defined(TARGET_X86) || defined(TARGET_ARM)
	// The x86 and arm32 CORINFO_HELP_INIT_PINVOKE_FRAME helpers have a custom calling convention.
	case WellKnownArg::PInvokeFrame:
	return REG_PINVOKE_FRAME;

After this PR, there is no usage of RBM_PINVOKE_FRAME, so can get rid of that as well.

Both are only used in x86/arm32, so the definitions outside could be removed. Do you want me to do that as part of this PR?

Yes, that will be good.

[jakobbotsch]

After this PR, there is no usage of RBM_PINVOKE_FRAME, so can get rid of that as well.

There are plenty of other cases where we have REG_X and RBM_X where only one is used, but where both are defined. E.g. REG_EXCEPTION_OBJECT (unused) vs RBM_EXCEPTION_OBJECT (used), REG_R2R_INDIRECT_PARAM (used) vs RBM_R2R_INDIRECT_PARAM (unused), etc.

Like I mentioned above I think we should consistently define both versions, so I don't think we should remove the RBM version. But I can remove the definitions outside x86/arm32.

[kunalspathak]

Like I mentioned above I think we should consistently define both versions, so I don't think we should remove the RBM version. But I can remove the definitions outside x86/arm32.

SGTM

[kunalspathak]

LGTM. Thanks!