Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: fix assert when there are mixed types in Enum.HasFlags optimization #23902

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 18 additions & 7 deletions src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12910,7 +12910,8 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp)
return nullptr;
}

GenTree* flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_REMOVE_BUT_NOT_NARROW);
// Do likewise with flagOp.
GenTree* flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_DONT_REMOVE);
if (flagVal == nullptr)
{
// Note we may fail here if the flag operand comes from
Expand All @@ -12919,19 +12920,29 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp)
return nullptr;
}

// Only proceed when both box sources have the same actual type.
// (this rules out long/int mismatches)
if (genActualType(thisVal->TypeGet()) != genActualType(flagVal->TypeGet()))
{
JITDUMP("bailing, pre-boxed values have different types\n");
return nullptr;
}

// Yes, both boxes can be cleaned up. Optimize.
JITDUMP("Optimizing call to Enum.HasFlag\n");

// Undo the boxing of thisOp and prepare to operate directly
// on the original enum values.
// Undo the boxing of the Ops and prepare to operate directly
// on the pre-boxed values.
thisVal = gtTryRemoveBoxUpstreamEffects(thisOp, BR_REMOVE_BUT_NOT_NARROW);
flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_REMOVE_BUT_NOT_NARROW);

// Our trial removal above should guarantee successful removal here.
// Our trial removals above should guarantee successful removals here.
assert(thisVal != nullptr);
assert(flagVal != nullptr);
assert(genActualType(thisVal->TypeGet()) == genActualType(flagVal->TypeGet()));

// We should have a consistent view of the type
var_types type = thisVal->TypeGet();
assert(type == flagVal->TypeGet());
// Type to use for optimized check
var_types type = genActualType(thisVal->TypeGet());

// The thisVal and flagVal trees come from earlier statements.
//
Expand Down
25 changes: 24 additions & 1 deletion tests/src/JIT/opt/Enum/hasflag.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// except for the case in the try/catch.

using System;
using System.Runtime.CompilerServices;

enum E
{
Expand Down Expand Up @@ -40,6 +41,12 @@ class EClass
public E e;
}

class ShortHolder
{
public ShortHolder(short s) { v = s; }
public short v;
}

class P
{
static E[] ArrayOfE = { E.RED, E.BLUE };
Expand Down Expand Up @@ -81,6 +88,20 @@ public static bool ByrefG(ref G e1, G e2)
return e1.HasFlag(e2);
}

// Example from GitHub 23847
[MethodImpl(MethodImplOptions.NoInlining)]
public static bool GitHub23847(E e1, short s)
{
return GitHub23847Aux(s).HasFlag(e1);
}

// Once this is inlined we end up with a short pre-boxed value
public static E GitHub23847Aux(short s)
{
ShortHolder h = new ShortHolder(s);
return (E)h.v;
}

public static int Main()
{
E e1 = E.RED;
Expand Down Expand Up @@ -120,8 +141,10 @@ public static int Main()
G g1 = G.RED;
bool true9 = ByrefG(ref g1, G.RED);

bool false10 = GitHub23847(E.RED, 0x100);

bool[] trueResults = {true0, true1, true2, true3, true4, true5, true6, true7, true8, true9};
bool[] falseResults = {false0, false1, false2, false3, false4, false5, false6, false7, false8, false9};
bool[] falseResults = {false0, false1, false2, false3, false4, false5, false6, false7, false8, false9, false10};

bool resultOk = true;

Expand Down
1 change: 0 additions & 1 deletion tests/src/JIT/opt/Enum/hasflag.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
<ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT .0\UITestExtensionPackages</ReferencePath>
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
<NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
Expand Down