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.
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
Generate branchless IL for (b ? 1 : 0)
#67191
Conversation
IL_0018: ldc.i4.0 | ||
IL_0019: ret | ||
IL_001a: ldc.i4.1 | ||
IL_001b: ret | ||
} | ||
"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness_Rebuild
fails with weird errors (AzDo build).
-
It crashes with OOM (repeatedly - can even reproduce that locally).
Details:
2023-03-07T14:25:15.5083562Z Validation failed for D:\a\_work\1\s\artifacts/obj/Microsoft.CodeAnalysis.ExpressionCompiler\Release\netstandard2.0\Microsoft.CodeAnalysis.ExpressionEvaluator.ExpressionCompiler.dll 2023-03-07T14:25:15.5084335Z Writing diffs to "D:\a\_work\1\s\artifacts\BuildValidator\netstandard2.0\Microsoft.CodeAnalysis.ExpressionEvaluator.ExpressionCompiler" 2023-03-07T14:25:16.6559691Z Exception of type 'System.OutOfMemoryException' was thrown. 2023-03-07T14:25:17.2868546Z Unhandled exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.BadImageFormatException: Exception of type 'System.OutOfMemoryException' was thrown. 2023-03-07T14:25:17.2912239Z at System.Reflection.PortableExecutable.PEReader.DecodeEmbeddedPortablePdbDebugDirectoryData(AbstractMemoryBlock block) 2023-03-07T14:25:17.2913380Z at System.Reflection.PortableExecutable.PEReader.ReadEmbeddedPortablePdbDebugDirectoryData(DebugDirectoryEntry entry) 2023-03-07T14:25:17.2914522Z at System.Reflection.PortableExecutable.PEReader.TryOpenEmbeddedPortablePdb(DebugDirectoryEntry embeddedPdbEntry, Boolean& openedEmbeddedPdb, MetadataReaderProvider& provider, Exception& errorToReport) 2023-03-07T14:25:17.2915533Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2916473Z at System.Reflection.PortableExecutable.PEReader.TryOpenAssociatedPortablePdb(String peImagePath, Func`2 pdbFileStreamProvider, MetadataReaderProvider& pdbReaderProvider, String& pdbPath) 2023-03-07T14:25:17.2917705Z at BuildValidator.Program.ValidateFile(Options options, AssemblyInfo assemblyInfo, ILogger logger, LocalReferenceResolver referenceResolver) in /_/src/Tools/BuildValidator/Program.cs:line 278 2023-03-07T14:25:17.2918862Z at BuildValidator.Program.ValidateFiles(IEnumerable`1 assemblyInfos, Options options, ILoggerFactory loggerFactory) in /_/src/Tools/BuildValidator/Program.cs:line 203 2023-03-07T14:25:17.2920092Z at BuildValidator.Program.HandleCommand(String[] assembliesPath, String[] exclude, String sourcePath, String[] referencesPath, Boolean verbose, Boolean quiet, Boolean debug, String debugPath) in /_/src/Tools/BuildValidator/Program.cs:line 140 2023-03-07T14:25:17.2921119Z --- End of inner exception stack trace --- 2023-03-07T14:25:17.2921791Z at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor) 2023-03-07T14:25:17.2922646Z at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments) 2023-03-07T14:25:17.2923305Z at System.Delegate.DynamicInvokeImpl(Object[] args) 2023-03-07T14:25:17.2923909Z at System.CommandLine.Invocation.ModelBindingCommandHandler.<InvokeAsync>d__12.MoveNext() 2023-03-07T14:25:17.2924532Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2925172Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2925877Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2926703Z at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext() 2023-03-07T14:25:17.2927388Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2928302Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2928941Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2929653Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseParseErrorReporting>b__21_0>d.MoveNext() 2023-03-07T14:25:17.2930293Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2930858Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2931547Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2932274Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseHelp>b__0>d.MoveNext() 2023-03-07T14:25:17.2933007Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2933583Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2934255Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2935036Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass25_0.<<UseVersionOption>b__0>d.MoveNext() 2023-03-07T14:25:17.2935862Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2936465Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2937091Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2937850Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass23_0.<<UseTypoCorrections>b__0>d.MoveNext() 2023-03-07T14:25:17.2938507Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2939096Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2939726Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2940461Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__22_0>d.MoveNext() 2023-03-07T14:25:17.2941157Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2941704Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2942359Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2943033Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseParseDirective>b__20_0>d.MoveNext() 2023-03-07T14:25:17.2943675Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2944209Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2944873Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2945555Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseDebugDirective>b__11_0>d.MoveNext() 2023-03-07T14:25:17.2946187Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2946767Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2947395Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 2023-03-07T14:25:17.2948145Z at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__10_0>d.MoveNext() 2023-03-07T14:25:17.2948773Z --- End of stack trace from previous location where exception was thrown --- 2023-03-07T14:25:17.2949358Z at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) 2023-03-07T14:25:17.2949983Z at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
-
Before it crashes,
BuildValidator.exe
also reports different IL after rebuild in test classes - seems exactly the test classes that this PR touches. Constants are emitted differently, I have no idea why.Details:
For example, in
PatternSwitchTests.DuplicateDouble
:- IL_000a: /* 23 | 40091EB851EB851F */ ldc.r8 3.1400000000000001 + IL_000a: /* 23 | 40091EB951EB851F */ ldc.r8 3.1400019073486329
In
AttributeTests.TestAttributesOnPropertyAndGetSet
:@@ -2331,7 +2331,7 @@ IL_00fc: /* A2 | */ stelem.ref IL_00fd: /* 25 | */ dup IL_00fe: /* 19 | */ ldc.i4.3 - IL_00ff: /* 23 | 400921FB4D12D84A */ ldc.r8 3.1415926000000001 + IL_00ff: /* 23 | 400921FC4D12D84A */ ldc.r8 3.1415945073486329 IL_0108: /* 8C | (01)0002D0 */ box [mscorlib/*23000001*/]System.Double/*010002D0*/ + "}\r\n1.0m !\r\nnull" /* 7036B889 */ - IL_0036: /* 23 | 4000CCCCCCCCCCCD */ ldc.r8 2.1000000000000001 + IL_0036: /* 23 | 4000CCCDCCCCCCCD */ ldc.r8 2.1000019073486329 IL_003f: /* 8C | (01)0002D0 */ box [mscorlib/*23000001*/]System.Double/*010002D0*/ IL_0044: /* 22 | 40066666 */ ldc.r4 2.0999999 IL_0049: /* 8C | (01)0002D1 */ box [mscorlib/*23000001*/]System.Single/*010002D1*/ @@ -595932,7 +595932,7 @@ IL_0050: /* 8D | (01)000014 */ newarr [mscorlib/*23000001*/]System.Object/*01000014*/ IL_0055: /* 25 | */ dup IL_0056: /* 16 | */ ldc.i4.0 - IL_0057: /* 23 | 3FF028F5C28F5C29 */ ldc.r8 1.01 + IL_0057: /* 23 | 3FF028F6C28F5C29 */ ldc.r8 1.0100009536743164 IL_0060: /* 0D | */ stloc.3 IL_0061: /* 12 | 03 */ ldloca.s V_3 IL_0063: /* 28 | (0A)0005CA */ call instance string [mscorlib/*23000001*/]System.Double/*010002D0*/::ToString() /* 0A0005CA */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try debugging that this morning and see if I come up with anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I figured out the OOM issue. The OOM is coming from a call to AllocHGlobal
deep inside the PEStream
loading code. The problem with AllocHGlobal
is that it's a native memory allocation function and hence doesn't represent any type of pressure to the GC. While we have memory available here, it's just not collected, there isn't enough to satisfy the 41MB request that happens because allocating native memory doesn't pressure the GC to do a collect.
I believe this is happening in your case because the earlier failures in the execution cause more memory to get loaded (to dump XML, MDV, etc ...). That pushes us over the limit and causes a crash.
Short term I fixed this by just making the app run as 64 bit locally. Still looking at the actual errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a test change to your PR and yes that fixed the issue. Will open a separate PR for that but will keep it here so it's not blocking you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the OOM. I see that the BuildValidator doesn't report any IL differences anymore - is that expected? I thought those two failures were unrelated, but maybe not.
This process uses a significant amount of memory as it's essentially doing compilation. Like the compiler server it needs to be 64 bit on 64 platforms to safely host all the items. Moved to explicit PlatformTarget of AnyCPU so it's not silently run as x86 in Debug.
I didn't understand some of the before/after assembly code in the PR description. For example, it looks like the Compare1 before and after are the same, but the performance is different. |
@jjonescz which runtime version were you benchmarking above? |
It was .NET 7. I added .NET 8 - but it seems the ASM is the same for this benchmark.
You're right, |
Updated benchmarks - the problem was having constant parameters - I randomized them. Also run them in a DevBox to avoid interference with my other work. |
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenConditionalOperatorTests.cs
Show resolved
Hide resolved
I think we should split the infrastructure changes into its own PR. |
Done with review pass (commit 5) |
It's in #67240. |
Done with review pass (commit 27) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 28), modulo the parameter name/documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the JIT side, we believe this will not cause perf regressions from older JITs - should be fine to do.
@dotnet/roslyn-compiler for a second review, please |
@@ -506,6 +500,64 @@ private void EmitCondExpr(BoundExpression condition, bool sense) | |||
return; | |||
} | |||
|
|||
/// <summary> | |||
/// Emits boolean expression without branching if possible (i.e., no logical operators, only comparisons). | |||
/// Leaves a boolean (int32) value on the stack which conforms to sense, i.e., <c>condition == sense</c>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth mentioning that the resulting value is either 0
or 1
only. #Closed
EmitIsExpression(isOp, used: true, omitBooleanConversion: true); | ||
|
||
// Convert to 1 or 0. | ||
_builder.EmitOpCode(ILOpCode.Ldnull); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow why we're pushing ldnull
here, but ldc_i4_0
below (line 543). When do we need to use one versus the other? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I found out. EmitIsExpression
uses Isinst
which returns "either a null reference or an instance of that class or interface."
} | ||
else if (condition is BoundIsOperator isOp) | ||
{ | ||
EmitIsExpression(isOp, used: true, omitBooleanConversion: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: EmitIsExpression
can already emit the ldnull
and cgt_un
. Did you consider adding a sense
parameter (which would drive cgt_un
or ceq
) instead of an omitBooleanConversion
parameter?
Update: I see that the current design allows to keep C# and VB changes more similar. Fine to keep as-is #Closed
{ | ||
if (expr.ConstantValueOpt is { } constantValue) | ||
{ | ||
if (constantValue is { IsIntegral: true, UInt64Value: (1 or 0) and var v }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3483,6 +3487,22 @@ private void EmitConditionalOperator(BoundConditionalOperator expr, bool used) | |||
{ | |||
Debug.Assert(expr.ConstantValueOpt == null, "Constant value should have been emitted directly"); | |||
|
|||
// Generate branchless IL for (b ? 1 : 0). | |||
if (used && _ilEmitStyle != ILEmitStyle.Debug && | |||
(IsNumeric(expr.Type) || expr.Type.PrimitiveTypeCode == Cci.PrimitiveTypeCode.Boolean) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, here:
roslyn/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenConditionalOperatorTests.cs
Lines 125 to 137 in e9aca83
Write(x <= y ? true : false); | |
Write(x <= y ? false : true); | |
Write(x != y ? (byte)1 : (byte)0); | |
Write(x != y ? 1 : (sbyte)0); | |
Write(x != y ? (short)1 : (short)0); | |
Write(x != y ? (ushort)1 : 0); | |
Write(x != y ? 1U : 0); | |
Write(x != y ? 1L : 0); | |
Write(x != y ? 1UL : 0); | |
Write(x != y ? (nint)1 : 0); | |
Write(x != y ? 1 : (nuint)0); | |
Write(x < y ? (char)0 : (char)1); | |
Write(x < y ? '\x1' : '\x0'); |
@@ -3483,6 +3487,22 @@ private void EmitConditionalOperator(BoundConditionalOperator expr, bool used) | |||
{ | |||
Debug.Assert(expr.ConstantValueOpt == null, "Constant value should have been emitted directly"); | |||
|
|||
// Generate branchless IL for (b ? 1 : 0). | |||
if (used && _ilEmitStyle != ILEmitStyle.Debug && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
@@ -15,6 +15,567 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests | |||
Public Class CodeGenIfOperator | |||
Inherits BasicTestBase | |||
|
|||
<Fact, WorkItem(61483, "https://github.com/dotnet/roslyn/issues/61483")> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider also testing with IntPtr in VB (optimization should not kick in, unlike C# in some cases) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (iteration 29). Only some test suggestions to consider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 33)
Resolves #61483.
Benchmarks (source)
Compare2 Release
Compare2 ReleaseCustomRoslyn
Compare3 Release
Compare3 ReleaseCustomRoslyn