[LuckyXu-HF]

• LoongArch ABI also requires the caller handling for small primitive arguments sign/zero-extend.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[shushanhf]

LGTM, thanks

[LuckyXu-HF]

Hi @jakobbotsch Could you please help to review this PR? Thanks.

[LuckyXu-HF]

Hi @jakobbotsch, the Runtime_101046.il appears to have been deliberately modified to trigger the test scenario, the reason for skipping this test on LA64 is that ldind.u2 should not be sign-extended semantically, from this perspective the test does not comply with LoongArch ABI, and we cannot guarantee its correctness in this scenario.

However, if this is intentionally a specific test and no need to consider semantics, we should take a fix like

runtime/src/coreclr/jit/importercalls.cpp

Lines 7523 to 7533 in e33ea58

	if ((compAppleArm64Abi() || TargetArchitecture::IsArm32 || TargetArchitecture::IsWasm) &&
	call->IsUnmanaged() && varTypeIsSmall(jitSigType))
	{
	// Wasm, Apple arm64 and arm32 ABIs require arguments to be zero/sign
	// extended up to 32 bit. The managed ABI does not require
	// this.
	if (fgCastNeeded(argNode, jitSigType))
	{
	argNode = gtNewCastNode(TYP_INT, argNode, false, jitSigType);
	}
	}

because LoongArch ABI requires the caller to handle sign/zero-extend.

Could you help to give us some suggestions? thank you very much!

[jakobbotsch]

Hi @jakobbotsch, the Runtime_101046.il appears to have been deliberately modified to trigger the test scenario, the reason for skipping this test on LA64 is that ldind.u2 should not be sign-extended semantically, from this perspective the test does not comply with LoongArch ABI, and we cannot guarantee its correctness in this scenario.

However, if this is intentionally a specific test and no need to consider semantics, we should take a fix like

runtime/src/coreclr/jit/importercalls.cpp

Lines 7523 to 7533 in e33ea58

	if ((compAppleArm64Abi() || TargetArchitecture::IsArm32 || TargetArchitecture::IsWasm) &&
	call->IsUnmanaged() && varTypeIsSmall(jitSigType))
	{
	// Wasm, Apple arm64 and arm32 ABIs require arguments to be zero/sign
	// extended up to 32 bit. The managed ABI does not require
	// this.
	if (fgCastNeeded(argNode, jitSigType))
	{
	argNode = gtNewCastNode(TYP_INT, argNode, false, jitSigType);
	}
	}

because LoongArch ABI requires the caller to handle sign/zero-extend.

Could you help to give us some suggestions? thank you very much!

If loongarch's native platform ABI requires callers to zero/sign extend small types, then indeed this should be changed to also kick in for loongarch.

[LuckyXu-HF]

Hi @jakobbotsch, the Runtime_101046.il appears to have been deliberately modified to trigger the test scenario, the reason for skipping this test on LA64 is that ldind.u2 should not be sign-extended semantically, from this perspective the test does not comply with LoongArch ABI, and we cannot guarantee its correctness in this scenario.
However, if this is intentionally a specific test and no need to consider semantics, we should take a fix like

runtime/src/coreclr/jit/importercalls.cpp

Lines 7523 to 7533 in e33ea58

	if ((compAppleArm64Abi() || TargetArchitecture::IsArm32 || TargetArchitecture::IsWasm) &&
	call->IsUnmanaged() && varTypeIsSmall(jitSigType))
	{
	// Wasm, Apple arm64 and arm32 ABIs require arguments to be zero/sign
	// extended up to 32 bit. The managed ABI does not require
	// this.
	if (fgCastNeeded(argNode, jitSigType))
	{
	argNode = gtNewCastNode(TYP_INT, argNode, false, jitSigType);
	}
	}

because LoongArch ABI requires the caller to handle sign/zero-extend.
Could you help to give us some suggestions? thank you very much!

If loongarch's native platform ABI requires callers to zero/sign extend small types, then indeed this should be changed to also kick in for loongarch.

Thank you very much, updated. Could you please help review this again?