Skip to content
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

Jump to bottom

LambdaExpression.CanCompileToIL should respect IsDynamicCodeSupported #80759

Merged
merged 2 commits into from
Jan 19, 2023
Merged

LambdaExpression.CanCompileToIL should respect IsDynamicCodeSupported #80759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cf354a
LambdaExpression.CanCompileToIL should respect IsDynamicCodeSupported
eerhardt Jan 18, 2023
807d807
Add tests
eerhardt Jan 18, 2023
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
6 changes: 3 additions & 3 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/LambdaExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public abstract class LambdaExpression : Expression, IParameterProvider
private readonly Expression _body;

// This can be flipped to false using feature switches at publishing time
public static bool CanCompileToIL => true;
public static bool CanCompileToIL => RuntimeFeature.IsDynamicCodeSupported;

// This could be flipped to false using feature switches at publishing time
public static bool CanInterpret => true;
Expand Down Expand Up @@ -154,7 +154,7 @@ public Delegate Compile()
/// <returns>A delegate containing the compiled version of the lambda.</returns>
public Delegate Compile(bool preferInterpretation)
{
if (CanCompileToIL && CanInterpret && preferInterpretation)
if (CanInterpret && preferInterpretation)
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if this impacts size with NativeAOT?

The dead branch elimination in NativeAOT only considers things that have feature switches. It doesn't attempt to inline things.

I think this is changing things from if (false && CanInterpret && preferInterpretation) to if (CanInterpret && preferInterpretation) (no longer a known constant).

Could be fixed by creating a substitution rule for CanInterpret as well.

(Either that or beefing up the dead branch elimination in NativeAOT. The dead branch elimination in NativeAOT is simpler than in linker because: 1. linker's one is super scary and I don't want to have to deal with the bugs in it that I would get by porting it, and 2. longer term, we should have RyuJIT do the dead branch elimination. RyuJIT is much better at it than anything we would write specifically for trimming.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe this is fine now that I look at it for longer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 reasons why I'm proposing this change.

  1. Logically, the condition doesn't make sense. Why would CanCompileToIL need to be true in order for preferInterpretation be respected?
  2. Looking at what it functionally does:

    runtime/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/LambdaExpression.cs

    Lines 137 to 163 in 7392fb7

    public Delegate Compile()
    {
    if (CanCompileToIL)
    {
    return Compiler.LambdaCompiler.Compile(this);
    }
    else
    {
    Debug.Assert(CanInterpret);
    return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
    }
    }
    /// <summary>
    /// Produces a delegate that represents the lambda expression.
    /// </summary>
    /// <param name="preferInterpretation">A <see cref="bool"/> that indicates if the expression should be compiled to an interpreted form, if available.</param>
    /// <returns>A delegate containing the compiled version of the lambda.</returns>
    public Delegate Compile(bool preferInterpretation)
    {
    if (CanCompileToIL && CanInterpret && preferInterpretation)
    {
    return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
    }
    return Compile();
    }

    If CanCompileToIL == false, the same code (return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();) is going to be executed regardless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it makes sense to do. I'm responsible for introducing this in #61952.

That pull request was a mechanical replacement of #defines. The code originally looked like:

        public Delegate Compile(bool preferInterpretation)
        {
#if FEATURE_COMPILE && FEATURE_INTERPRET
            if (preferInterpretation)
            {
                return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
            }
#endif
            return Compile();
        }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second look, the original code doesn't make that much more sense either...

{
return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
}
Expand Down Expand Up @@ -234,7 +234,7 @@ internal Expression(Expression body)
/// <returns>A delegate containing the compiled version of the lambda.</returns>
public new TDelegate Compile(bool preferInterpretation)
{
if (CanCompileToIL && CanInterpret && preferInterpretation)
if (CanInterpret && preferInterpretation)
{
return (TDelegate)(object)new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
}
Expand Down
24 changes: 24 additions & 0 deletions src/libraries/System.Linq.Expressions/tests/CompilerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Runtime.CompilerServices;
using System.Text.RegularExpressions;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

namespace System.Linq.Expressions.Tests
Expand Down Expand Up @@ -429,6 +430,29 @@ private static void Verify_VariableBinder_CatchBlock_Filter(CatchBlock @catch)

Assert.Throws<InvalidOperationException>(() => e.Compile());
}

/// <summary>
/// Verifies that compiling and executing a lambda method works when IsDynamicCodeSupported == false.
/// </summary>
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void CompileWorksWhenDynamicCodeNotSupported()
{
RemoteInvokeOptions options = new RemoteInvokeOptions();
options.RuntimeConfigurationOptions.Add("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", false.ToString());

using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(static () =>
{
ParameterExpression param = Expression.Parameter(typeof(int));
Func<int, int> typedDel =
Expression.Lambda<Func<int, int>>(Expression.Add(param, Expression.Constant(4)), param).Compile();
Assert.Equal(304, typedDel(300));
Delegate del =
Expression.Lambda(Expression.Add(param, Expression.Constant(5)), param).Compile();
Assert.Equal(305, del.DynamicInvoke(300));
}, options);
}
}

public enum ConstantsEnum
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Linq.Expressions/tests/System.Linq.Expressions.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-maccatalyst</TargetFrameworks>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
</PropertyGroup>

<ItemGroup>
Expand Down