Add back missing conv opcodes when compiling via System.Linq.Expressions

[BrzVlad]

The conversion opcodes are still necessary when the sign of the value might change, in which case the conversion opcode will do a sign/zero extend to the full i32 storage used by the IL execution stack.

For example, before this change, conversions from ushort to short were ignored. Consider expressions converting the value ushort.MaxValue to short (testcase ConvertUShortToShortTest). ushort.MaxValue will be pushed to execution stack as a i32 ldc of value 0xffff. The conv.i2 opcode would change the value on the stack to 0xffffffff so it shouldn't be omitted.

[ghost]

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

Issue Details

---

The conversion opcodes are still necessary when the sign of the value might change, in which case the conversion opcode will do a sign/zero extend to the full i32 storage used by the IL execution stack.

For example, before this change, conversions from ushort to short were ignored. Consider expressions converting the value ushort.MaxValue to short (testcase ConvertUShortToShortTest). ushort.MaxValue will be pushed to execution stack as a i32 ldc of value 0xffff. The conv.i2 opcode would change the value on the stack to 0xffffffff so it shouldn't be omitted.

Author:	BrzVlad
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[BrzVlad]

While omitting conversions seems to work fine on coreclr and was also working fine on mono, a mono interpreter optimization to reduce redundant conversions (#75837) hit this issue.

The conv omission originates from 3d4b84e

[stephentoub]

Are there tests you can add? Or do we already have such tests and those tests were indeed failing on the interpreter?

[BrzVlad]

@stephentoub This issue was hit with already existing SLE tests on interpreter, so I think this is good enough for such a small change.

[jkotas]

Could you please share a minimal IL repro that demonstrates the problem?

I am not aware of any ambiguity in the ECMA spec about how to represent the small integer types on the IL stack; or how these values should behave with conversions. If the tests are only failing on interpreter, it sounds like there is an bug in the interpreter that should be fixed. We should not be compensating for it by emitting different IL.

[BrzVlad]

@jkotas @stephentoub Ok, maybe this issue does need some tests. My change is not really about adding a conversion opcode because mono interpreter is not behaving accordingly (even though this more or less describes how I stumbled across this issue). I opened this PR because SLE ILGen is ignoring certain conversions for what I consider to be dubious reasons. Take this sample for example:

using System;
using System.Linq.Expressions;
using System.Runtime.CompilerServices;

public class Program {

	[MethodImpl(MethodImplOptions.NoInlining)]
	public static int ConvI2 (ushort val)
	{
		short vals = (short)val;
		return (int)vals;
	}

	public static Func<ushort, int> GetConvFunc (bool interpret)
	{
		ParameterExpression param = Expression.Parameter (typeof(ushort));
		UnaryExpression convert = Expression.Convert (param, typeof(short));
		UnaryExpression converti = Expression.Convert (convert, typeof(int));

		Expression<Func<ushort, int>> e = Expression.Lambda<Func<ushort, int>>(converti, new ParameterExpression [] { param });
		Func<ushort, int> f = e.Compile(interpret);
		return f;
	}

	public static int ConvI2SLECompiled (ushort val)
	{
		return GetConvFunc (false)(val);
	}

	public static int ConvI2SLEInterpreted (ushort val)
	{
		return GetConvFunc (true)(val);
	}

	public static void Main (string[] args)
	{
		Console.WriteLine (ConvI2 (ushort.MaxValue));
		Console.WriteLine (ConvI2SLECompiled (ushort.MaxValue));
		Console.WriteLine (ConvI2SLEInterpreted (ushort.MaxValue));
	}
}

With all execution engines, both CoreCLR and mono, this prints.

-1
65535
-1

The IL of method ConvI2 is

IL_0000:  ldarg.0
IL_0001:  conv.i2
// conv.i4 is nop
IL_0002:  ret

The test is attempting to generate the same code via the linq expressions, but the resulting compiled dynamic method skips the conv.i2 opcode and is just

IL_0000: ldarg     1
IL_0004: nop       
IL_0005: nop       
IL_0006: ret 

And the conv.i2 opcode is not a nop because, as you can see, the result of the method is different between the compiled and interpreted versions.

[jkotas]

Ah ok, you meant System.Linq.Expressions interpreter.

What are the existing System.Linq.Expressions tests that are hitting this issue?

All System.Linq.Expressions tests should be running both with preferInterpretation: true and preferInterpretation: false and none of them are failing. If the existing SLE tests were hitting this issue, we should see them failing everywhere. I think we need a new SLE test added for this that actually hits the issues.

[BrzVlad]

Well, there is no issue with SLE interpreter only with SLE ILGen. I noticed this issue while running a tweaked version of the mono interpreter on SLE tests. There are no test failures on CI most likely because the return of the lambda expression is still a short (System.Linq.Expressions.Tests.ConvertTests.VerifyUShortToShort), instead of an int32 (as in my test case). When returning a short integer, the spec requires implicit conversions of the top of the stack to short and back to i32 on the execution stack of the calling method, hiding this issue. (The failure was happening on mono interpreter because it is missing these implicit return conversions)

[jkotas]

Could you please turn the sample from #76024 (comment) into a test that is failing before this change and passing after this change?

[jkotas]

LGTM. Thanks!