Skip to content

Preserve operand storage for ref conversion methods #624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jogibear9988 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Preserve operand storage for ref conversion methods #624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 16 additions & 21 deletions src/FastExpressionCompiler/FastExpressionCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3947,34 +3947,27 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com
// see #73, #451 for examples
Type methodReturnType = null;
Type methodParamType = null;
var liftedByRefMethod = false;
var methodParameterIsByRef = false;
if (convertMethod != null)
{
methodReturnType = convertMethod.ReturnType;

var mParams = convertMethod.GetParameters();
Debug.Assert(mParams.Length == 1, $"Expected for the conversion operator to have a single param, but found {mParams.Length}");
methodParamType = mParams[0].ParameterType;
if (methodParamType.IsByRef && sourceType.IsNullable() && methodParamType.GetElementType() != sourceType)
{
liftedByRefMethod = true;
methodParameterIsByRef = methodParamType.IsByRef;
if (methodParameterIsByRef)
methodParamType = methodParamType.GetElementType();
}

// todo: @wip check if we need to add the ParentFlags.Call here?
var isByRefParam = methodParamType.IsByRef;
var operandFlags = liftedByRefMethod
? (parent & ~CallArgumentFlagsToClear) | ParentFlags.AssignmentRightValue
: isByRefParam
? parent & ~ParentFlags.IgnoreResult & ~ParentFlags.InstanceAccess
: (parent & ~ValueOperandFlagsToClear) | ParentFlags.AssignmentRightValue;
if (!TryEmit(opExpr, il, ref context, operandFlags,
isByRefParam && !(opExpr is ParameterExpression { IsByRef: true } && sourceType.IsNullable()) ? 0 : -1))
var passSourceByRef = methodParameterIsByRef && methodParamType == sourceType;
var operandFlags = passSourceByRef
? (parent & ~(ParentFlags.IgnoreResult | ParentFlags.InstanceAccess | ParentFlags.MemberAccess |
ParentFlags.AssignmentRightValue | ParentFlags.Arithmetic | ParentFlags.LambdaCall | ParentFlags.ReturnByRef)) | ParentFlags.Call
: (parent & ~(ParentFlags.IgnoreResult | ParentFlags.InstanceAccess | ParentFlags.MemberAccess |
ParentFlags.AssignmentLeftValue | ParentFlags.AssignmentByRef | ParentFlags.DupIt |
ParentFlags.LambdaCall | ParentFlags.ReturnByRef)) | ParentFlags.AssignmentRightValue;
if (!TryEmit(opExpr, il, ref context, operandFlags, passSourceByRef ? 0 : -1))
return false;

// The by-ref param of the method needs the address of the operand, the nullable by-ref parameter is already the address
if (isByRefParam && !context.LastEmitIsAddress && !(opExpr is ParameterExpression { IsByRef: true }))
EmitStoreAndLoadLocalVariableAddress(il, sourceType);
}
else
{
Expand Down Expand Up @@ -4053,9 +4046,9 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com
il.DmarkLabel(labelSourceHasValue);
EmitLoadLocalVariableAddress(il, sourceVarIndex);
il.Demit(OpCodes.Ldfld, sourceType.GetNullableValueUnsafeAkaGetValueOrDefaultMethod());
if (liftedByRefMethod)
EmitStoreAndLoadLocalVariableAddress(il, methodParamType);

if (methodParameterIsByRef)
EmitStoreAndLoadLocalVariableAddress(il, methodParamType);
EmitMethodCallOrVirtualCall(il, convertMethod);

// Wrap the conversion result into the target type only if the conversion method return the underlying target type,
Expand All @@ -4064,6 +4057,7 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com
il.Demit(OpCodes.Newobj, targetType.GetNullableConstructor());

il.DmarkLabel(labelConversionDone);
context.LastEmitIsAddress = false;
return il.EmitPopIfIgnoreResult(parent);
}

Expand All @@ -4090,9 +4084,10 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com
$"Expected the conversion operator parameter type `{methodParamType}` to be either Nullable<{sourceType}> or a base type of `{sourceType}`");
}

if (liftedByRefMethod)
if (methodParameterIsByRef && methodParamType != sourceType)
EmitStoreAndLoadLocalVariableAddress(il, methodParamType);
EmitMethodCallOrVirtualCall(il, convertMethod);
context.LastEmitIsAddress = false;

if (methodReturnType == underlyingNullableTargetType)
il.Demit(OpCodes.Newobj, targetType.GetNullableConstructor());
Expand Down
1 change: 1 addition & 0 deletions test/FastExpressionCompiler.TestsRunner.Net472/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ static void RunLightExpressionTests(object state)
fecTests.Run(new ArrayBoundsExceptionRegressionTests());
fecTests.Run(new TryResultValueRegressionTests());
fecTests.Run(new NonVectorArrayRegressionTests());
fecTests.Run(new RefConversionStorageTests());
fecTests.Run(new UnboxStorageRegressionTests());
fecTests.Run(new ArrayInitializerExceptionRegressionTests());
fecTests.Run(new ReadonlyRefFieldRegressionTests());
Expand Down
1 change: 1 addition & 0 deletions test/FastExpressionCompiler.TestsRunner/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ public static void Main(string[] args)
st.Run(new ArrayBoundsExceptionRegressionTests());
st.Run(new TryResultValueRegressionTests());
st.Run(new NonVectorArrayRegressionTests());
st.Run(new RefConversionStorageTests());
st.Run(new UnboxStorageRegressionTests());
st.Run(new ArrayInitializerExceptionRegressionTests());
st.Run(new ReadonlyRefFieldRegressionTests());
Expand Down
106 changes: 106 additions & 0 deletions test/FastExpressionCompiler.UnitTests/RefConversionStorageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
#if !LIGHT_EXPRESSION
using System;
using System.Linq.Expressions;
using System.Reflection;
namespace FastExpressionCompiler.UnitTests;
public struct RefConversionStorageTests : ITestX
{
public void Run(TestRun t)
{
Ref_method_mutates_ref_operand(t);
Lifted_ref_method_uses_copy_and_throws_on_null(t);
Ref_method_handles_other_operand_locations(t);
Nonref_method_still_dereferences_ref_operand(t);
Lifted_ref_method_returns_nullable_struct(t);
Ignored_ref_conversion_keeps_mutation(t);
}
public void Ref_method_mutates_ref_operand(TestContext t) => Check(t, false, 0, nameof(ConvertRef));
public void Lifted_ref_method_uses_copy_and_throws_on_null(TestContext t) => Check(t, true, 0, nameof(ConvertRef));
public void Ref_method_handles_other_operand_locations(TestContext t)
{ for (var shape = 1; shape != 6; ++shape) { Check(t, false, shape, nameof(ConvertRef)); Check(t, true, shape, nameof(ConvertRef)); } }
public void Nonref_method_still_dereferences_ref_operand(TestContext t)
{ Check(t, false, 0, nameof(ConvertValue)); Check(t, true, 0, nameof(ConvertValue)); }
public void Lifted_ref_method_returns_nullable_struct(TestContext t) => Check(t, true, 0, nameof(ConvertStructRef));
public void Ignored_ref_conversion_keeps_mutation(TestContext t)
{
var p = Expression.Parameter(typeof(int).MakeByRefType());
var expression = Expression.Lambda<Mutation<int>>(Expression.Block(
Expression.Convert(p, typeof(Converted), typeof(RefConversionStorageTests).GetMethod(nameof(ConvertRef))), Expression.Constant(7)), p);
var fast = expression.CompileFast(true);
if (!t.IsNotNull(fast)) return;
var value = 5;
t.AreEqual(7, fast(ref value));
t.AreEqual(6, value);
}
private static void Check(TestContext t, bool nullable, int shape, string methodName)
{
var type = nullable ? typeof(int?) : typeof(int);
var p = Expression.Parameter(shape == 0 ? type.MakeByRefType() : type);
var local = Expression.Variable(type);
var cell = Expression.Variable(typeof(Cell<>).MakeGenericType(type));
Expression location = shape == 2 ? local : shape == 3 ? Expression.Field(cell, nameof(Cell<int>.Value))
: shape == 4 ? Expression.Constant(nullable ? (object)(int?)5 : 5, type)
: shape == 5 ? Expression.Call(typeof(RefConversionStorageTests).GetMethod(nullable ? nameof(ReadNullableOperand) : nameof(ReadOperand))) : p;
var method = typeof(RefConversionStorageTests).GetMethod(methodName);
var targetType = methodName == nameof(ConvertStructRef) ? typeof(ConvertedStruct?) : typeof(Converted);
var result = Expression.NewArrayInit(typeof(object), Expression.Convert(Expression.Convert(location, targetType, method), typeof(object)), Expression.Convert(shape >= 4 ? p : location, typeof(object)));
var body = shape == 2 ? Expression.Block(new[] { local }, Expression.Assign(local, p), result)
: shape == 3 ? Expression.Block(new[] { cell }, Expression.Assign(cell, Expression.New(cell.Type)), Expression.Assign(location, p), result) : (Expression)result;
var expression = shape == 0 ? Expression.Lambda(typeof(Read<>).MakeGenericType(type), body, p) : Expression.Lambda(body, p);
var fast = expression.CompileFast(true);
if (!t.IsNotNull(fast)) return;
foreach (var value in nullable ? new object[] { null, 5 } : new object[] { 5 })
{
var expectedArgs = new[] { value };
var actualArgs = new[] { value };
OperandCalls = 0;
#if NETFRAMEWORK
// The Framework expression compiler rejects conversion methods with ref parameters.
// Execute the same conversion directly to retain the value, mutation and operand-effect oracle.
var expected = EvaluateExpectedConversion(value, nullable, shape, methodName, expectedArgs);
#else
var expected = Invoke(expression.Compile(), expectedArgs);
#endif
var expectedCalls = OperandCalls;
OperandCalls = 0;
var actual = Invoke(fast, actualArgs);
if (expected is object[] e && actual is object[] a)
{
if (e[0] is Converted ec && a[0] is Converted ac) t.AreEqual(ec.Value, ac.Value);
else t.AreEqual(e[0], a[0]);
t.AreEqual(e[1], a[1]);
}
else t.AreEqual(expected, actual);
t.AreEqual(expectedArgs[0], actualArgs[0]);
t.AreEqual(expectedCalls, OperandCalls);
}
}
#if NETFRAMEWORK
private static object EvaluateExpectedConversion(object value, bool nullable, int shape, string methodName, object[] arguments)
{
var operand = shape == 4 ? 5 : shape == 5 ? nullable ? (object)ReadNullableOperand() : ReadOperand() : value;
if (operand == null)
return methodName == nameof(ConvertStructRef) ? (object)new object[] { null, value } : typeof(InvalidOperationException);
var temporary = (int)operand;
object converted = methodName == nameof(ConvertValue) ? (object)ConvertValue(temporary)
: methodName == nameof(ConvertStructRef) ? ConvertStructRef(ref temporary) : (object)ConvertRef(ref temporary);
var writesBack = !nullable && methodName != nameof(ConvertValue) && shape < 4;
if (writesBack && shape == 0) arguments[0] = temporary;
return new object[] { converted, writesBack ? temporary : value };
}
#endif
private static object Invoke(Delegate function, object[] arguments)
{ try { return function.DynamicInvoke(arguments); } catch (TargetInvocationException e) { return e.InnerException.GetType(); } }
public delegate object[] Read<T>(ref T value);
public delegate int Mutation<T>(ref T value);
public static int OperandCalls;
public static int ReadOperand() { ++OperandCalls; return 5; }
public static int? ReadNullableOperand() { ++OperandCalls; return 5; }
public static Converted ConvertRef(ref int value) => new Converted { Value = value++ };
public static Converted ConvertValue(int value) => new Converted { Value = value };
public static ConvertedStruct ConvertStructRef(ref int value) => new ConvertedStruct { Value = value++ };
public class Converted { public int Value; }
public struct ConvertedStruct { public int Value; }
public class Cell<T> { public T Value; }
}
#endif
Loading