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.

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

Merged
merged 34 commits into from Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
26b944a
Add tests
jjonescz Mar 6, 2023
e2c3f4d
Generate branchless IL for `(b ? 1 : 0)`
jjonescz Mar 6, 2023
01f7f64
Update IL snapshots
jjonescz Mar 6, 2023
333d1aa
Move BuildValidator to 64 bit
jaredpar Mar 8, 2023
1fe26ef
Rename `isOne` out parameter
jjonescz Mar 9, 2023
87a5313
Add nonbinary test
jjonescz Mar 13, 2023
bb482c5
Restrict the optimization
jjonescz Mar 13, 2023
513097c
Test IL in Debug
jjonescz Mar 13, 2023
62cc9d9
Encapsulate the emit into method
jjonescz Mar 14, 2023
50d7ae7
Convert result
jjonescz Mar 14, 2023
f5bda52
Disable optimization in Debug
jjonescz Mar 14, 2023
270c816
Improve wording
jjonescz Mar 14, 2023
6344f0c
Check type
jjonescz Mar 14, 2023
97e88fa
Rename emit comparison method
jjonescz Mar 15, 2023
3f19b46
Clarify test options
jjonescz Mar 15, 2023
9bee6a7
Extend tests
jjonescz Mar 15, 2023
3270493
Revert "Move BuildValidator to 64 bit"
jjonescz Mar 15, 2023
853b7bf
Merge branch 'main' into 61483-Emit-cgt
jjonescz Mar 15, 2023
49e600c
Optimize `char`s
jjonescz Mar 16, 2023
435381b
Remove unnecessary `char` check
jjonescz Mar 17, 2023
9c45b18
Extend `char` tests
jjonescz Mar 17, 2023
08e5bf0
Add VB tests
jjonescz Mar 20, 2023
3a4b3c4
Emit branchless IL for `If(b, 1, 0)` in VB
jjonescz Mar 20, 2023
4e09301
Update IL snapshot
jjonescz Mar 20, 2023
ff8b135
Add sequence points to IL snapshots
jjonescz Mar 21, 2023
65e3db4
Test `isinst`
jjonescz Mar 21, 2023
857b0c4
Optimize `isinst`
jjonescz Mar 21, 2023
1f0cc3a
Optimize `is` codegen when used inside condition
jjonescz Mar 21, 2023
e9aca83
Rename parameter which omits boolean conversion of `is` codegen
jjonescz Mar 22, 2023
f2c8d13
Clarify return values
jjonescz Apr 14, 2023
be1432c
Improve variable naming
jjonescz Apr 14, 2023
d202e2b
Test IntPtr in VB
jjonescz Apr 14, 2023
c3e6928
Test `ILEmitStyle.DebugFriendlyRelease`
jjonescz Apr 14, 2023
72b4890
Merge branch 'main' into 61483-Emit-cgt
jjonescz Apr 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 51 additions & 4 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
Expand Up @@ -202,7 +202,7 @@ private void EmitExpressionCore(BoundExpression expression, bool used)
break;

case BoundKind.IsOperator:
EmitIsExpression((BoundIsOperator)expression, used);
EmitIsExpression((BoundIsOperator)expression, used, omitBooleanConversion: false);
break;

case BoundKind.AsOperator:
Expand Down Expand Up @@ -3158,7 +3158,7 @@ private void EmitPopIfUnused(bool used)
}
}

private void EmitIsExpression(BoundIsOperator isOp, bool used)
private void EmitIsExpression(BoundIsOperator isOp, bool used, bool omitBooleanConversion)
{
var operand = isOp.Operand;
EmitExpression(operand, used);
Expand All @@ -3172,8 +3172,12 @@ private void EmitIsExpression(BoundIsOperator isOp, bool used)
}
_builder.EmitOpCode(ILOpCode.Isinst);
EmitSymbolToken(isOp.TargetType.Type, isOp.Syntax);
_builder.EmitOpCode(ILOpCode.Ldnull);
_builder.EmitOpCode(ILOpCode.Cgt_un);

if (!omitBooleanConversion)
{
_builder.EmitOpCode(ILOpCode.Ldnull);
_builder.EmitOpCode(ILOpCode.Cgt_un);
}
}
}

Expand Down Expand Up @@ -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 &&
Copy link
Member

@jcouv jcouv Apr 12, 2023

Choose a reason for hiding this comment

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

_ilEmitStyle != ILEmitStyle.Debug

You mentioned DebugFriendlyRelease earlier. Do we have a test for that emit style? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

(IsNumeric(expr.Type) || expr.Type.PrimitiveTypeCode == Cci.PrimitiveTypeCode.Boolean) &&
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@jcouv jcouv Apr 12, 2023

Choose a reason for hiding this comment

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

IsNumeric(expr.Type)

Do we have tests for each of the numeric types? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here:

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');

hasIntegralValueZeroOrOne(expr.Consequence, out var isConsequenceOne) &&
hasIntegralValueZeroOrOne(expr.Alternative, out var isAlternativeOne) &&
isConsequenceOne != isAlternativeOne &&
TryEmitComparison(expr.Condition, sense: isConsequenceOne))
{
var toType = expr.Type.PrimitiveTypeCode;
if (toType != Cci.PrimitiveTypeCode.Boolean)
{
_builder.EmitNumericConversion(Cci.PrimitiveTypeCode.Int32, toType, @checked: false);
}
return;
}

object consequenceLabel = new object();
object doneLabel = new object();

Expand Down Expand Up @@ -3547,6 +3567,33 @@ private void EmitConditionalOperator(BoundConditionalOperator expr, bool used)
}

_builder.MarkLabel(doneLabel);

static bool hasIntegralValueZeroOrOne(BoundExpression expr, out bool isOne)
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
{
if (expr.ConstantValueOpt is { } constantValue)
{
if (constantValue is { IsIntegral: true, UInt64Value: (1 or 0) and var i })
{
isOne = i == 1;
return true;
}

if (constantValue is { IsBoolean: true, BooleanValue: var b })
{
isOne = b;
return true;
}

if (constantValue is { IsChar: true, CharValue: ((char)1 or (char)0) and var c })
{
isOne = c == 1;
return true;
}
}

isOne = false;
return false;
}
}

/// <summary>
Expand Down
66 changes: 59 additions & 7 deletions src/Compilers/CSharp/Portable/CodeGen/EmitOperators.cs
Expand Up @@ -471,13 +471,7 @@ private void EmitBinaryCondOperatorHelper(ILOpCode opCode, BoundExpression left,
// this will leave a value on the stack which conforms to sense, ie:(condition == sense)
private void EmitCondExpr(BoundExpression condition, bool sense)
{
while (condition.Kind == BoundKind.UnaryOperator)
{
var unOp = (BoundUnaryOperator)condition;
Debug.Assert(unOp.OperatorKind == UnaryOperatorKind.BoolLogicalNegation);
condition = unOp.Operand;
sense = !sense;
}
RemoveNegation(ref condition, ref sense);

Debug.Assert(condition.Type.SpecialType == SpecialType.System_Boolean);

Expand Down Expand Up @@ -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, 0 or 1) value on the stack which conforms to sense, i.e., <c>condition == sense</c>.
/// </summary>
private bool TryEmitComparison(BoundExpression condition, bool sense)
{
RemoveNegation(ref condition, ref sense);
jjonescz marked this conversation as resolved.
Show resolved Hide resolved

Debug.Assert(condition.Type.SpecialType == SpecialType.System_Boolean);

if (condition.ConstantValueOpt is { } constantValue)
{
Debug.Assert(constantValue.Discriminator == ConstantValueTypeDiscriminator.Boolean);
_builder.EmitBoolConstant(constantValue.BooleanValue == sense);
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

if (condition is BoundBinaryOperator binOp)
{
// Intentionally don't optimize logical operators, they need branches to short-circuit.
if (binOp.OperatorKind.IsComparison())
{
EmitBinaryCondOperator(binOp, sense: sense);
return true;
}
}
else if (condition is BoundIsOperator isOp)
{
EmitIsExpression(isOp, used: true, omitBooleanConversion: true);
Copy link
Member

@jcouv jcouv Apr 12, 2023

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


// Convert to 1 or 0.
_builder.EmitOpCode(ILOpCode.Ldnull);
Copy link
Member

@jcouv jcouv Apr 12, 2023

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

Copy link
Member

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."

_builder.EmitOpCode(sense ? ILOpCode.Cgt_un : ILOpCode.Ceq);
return true;
}
else
{
EmitExpression(condition, used: true);

// Convert to 1 or 0 (although `condition` is of type `bool`, it can contain any integer).
_builder.EmitOpCode(ILOpCode.Ldc_i4_0);
_builder.EmitOpCode(sense ? ILOpCode.Cgt_un : ILOpCode.Ceq);
return true;
}

return false;
}

private static void RemoveNegation(ref BoundExpression condition, ref bool sense)
{
while (condition is BoundUnaryOperator unOp)
{
Debug.Assert(unOp.OperatorKind == UnaryOperatorKind.BoolLogicalNegation);
condition = unOp.Operand;
sense = !sense;
}
}

private void EmitUnaryCheckedOperatorExpression(BoundUnaryOperator expression, bool used)
{
Debug.Assert(expression.OperatorKind.Operator() == UnaryOperatorKind.UnaryMinus);
Expand Down