JIT: optimize Enum.HasFlag by AndyAyersMS · Pull Request #13748 · dotnet/coreclr

AndyAyersMS · 2017-09-01T17:26:29Z

In the vm, add mscorlib binder support for Enum.HasFlag. Recognize this
method as a may-expand intrinsic.

In the jit, generalize the behavior of gtTryRemoveBoxUpstreamEffects to
add a "trial removal" mode and to optionally suppress narrowing of the
copy source.

Then implement the optimization by checking for the intrinsic and ensuring
that both operands are boxes with compatible types that can be removed.
The optimization changes the call to a simple and/compare tree on the
underlying enum values.

Invoke the optimzation during importation (which will catch most cases) and
again during morph (to get the post-inline cases).

Currently not thinking of bumping the JIT GUID since this is a may expand
intrinsic and no existing intrinsic ID has changed.

Added test case. Suprisingly there were almost no uses of HasFlag in the
current CoreCLR test suite.

Closes #5626.

AndyAyersMS · 2017-09-01T17:30:52Z

@jkotas PTAL (vm parts at least)
@JosephTremoulet PTAL (jit parts)
cc @dotnet/jit-contrib

No jit-diffs impact,. There was one use of HasFlag in the test suite in baseservices\threading\commitstackonlyasneeded\DefaultStackCommit and the optimization fires on it when jitting:

;; Before

       48B980B284EAFD7F0000 mov      rcx, 0x7FFDEA84B280
       E81A07865F           call     CORINFO_HELP_NEWSFAST
       4C8BF8               mov      r15, rax
       8B8C2480000000       mov      ecx, dword ptr [rsp+80H]
       41894F08             mov      dword ptr [r15+8], ecx
       48B980B284EAFD7F0000 mov      rcx, 0x7FFDEA84B280
       E8FD06865F           call     CORINFO_HELP_NEWSFAST
       488BD0               mov      rdx, rax
       C7420800100000       mov      dword ptr [rdx+8], 0x1000
       498BCF               mov      rcx, r15
       E85B2DA75E           call     System.Enum:HasFlag(ref):bool:this
       85C0                 test     eax, eax
       0F857F000000         jne      G_M54767_IG12

;; After

       8B8C2480000000       mov      ecx, dword ptr [rsp+80H]
       F7C100100000         test     ecx, 0x1000
       754C                 jne      SHORT G_M54767_IG12

Will do some corefx testing too since there is likely better coverage there.

@dotnet-bot test Windows_NT x64 corefx_baseline

JosephTremoulet

Great to see this getting implemented, thanks!

JosephTremoulet · 2017-09-01T17:55:23Z

    GenTreePtr gtFoldExprCompare(GenTreePtr tree);
-    bool gtTryRemoveBoxUpstreamEffects(GenTreePtr tree);
+
+    enum BoxRemovalOptions


Could use a comment

Yup. Will add some.

JosephTremoulet · 2017-09-01T17:57:34Z

+    GenTree* asgStmt = op->gtBox.gtAsgStmtWhenInlinedBoxValue;
    assert(asgStmt->gtOper == GT_STMT);
-    GenTreePtr copyStmt = op->gtBox.gtCopyStmtWhenInlinedBoxValue;
+    GenTree* copyStmt = op->gtBox.gtCopyStmtWhenInlinedBoxValue;


Why GenTreePtr -> GenTree*?

Decided not to introduce new GenTreePtr and then having a mixture looked odd. So changed them all.

JosephTremoulet · 2017-09-01T18:03:04Z

+
+    // Pop the stack for real.
+    // impPopStack();
+    // impPopStack();


Looks like you do this in the importer now when calling from the importer, so can remove this commented code.

Thanks, yeah a vestige of the initial importer-only version.

JosephTremoulet · 2017-09-01T18:04:28Z

+        return nullptr;
+    }
+
+    GenTree* flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_DONT_REMOVE);


I'd be inclined to go ahead and use BR_REMOVE_BUT_NOT_NARROW here (and skip the call below), despite the asymmetry.

Good idea. Originally there was one more downstream check so both removals needed to be trials.

JosephTremoulet · 2017-09-01T18:10:43Z

+// See the LICENSE file in the project root for more information.
+
+// Some simple tests for the Enum.HasFlag optimization.
+// All the calls to HasFlag should be optimized to simple compares.


And are they all optimized with this code, or is there more to do for some of them?

All are now optimized (initially the ones with calls weren't). I'll fix the comment to be clearer.

mikedn · 2017-09-01T18:16:19Z

+    // The thisVal and flagVal trees come from earlier statements. We need
+    // to evaluate them both to temps at those points to safely transmit
+    // the values here.
+    const unsigned flagTmp         = lvaGrabTemp(true DEBUGARG("Enum:HasFlag flag temp"));


I guess it's not worth the trouble of handling the common case where flagVal is a constant and doesn't require a new temp.

I think it may be worth it... will give it a try.

CarolEidt

LGTM

CarolEidt · 2017-09-01T18:57:59Z

    {
-        printf("\nAttempting to remove side effects of BOX (valuetype)\n");
+        printf("\n%s remove side effects of BOX (valuetype)\n",
+               options == BR_DONT_REMOVE ? "Checking if it is possible" : "Attempting to");


Nit: either move the "to " from "Attempting to" to before "remove" , or add after "possible".

Fixed now. Thanks.

AndyAyersMS · 2017-09-01T19:34:33Z

Pushed an update (but missed Carol's note). Now in cases like

    public static int Main()
    {
        if (E.RED.HasFlag(E.BLUE))
        {
            Console.WriteLine("This bit should not be imported!\n");
        }
        return 100;
    }

The test is folded in the importer.

AndyAyersMS · 2017-09-01T19:35:09Z

Making sure this still passes: @dotnet-bot test Windows_NT x64 corefx_baseline

AndyAyersMS · 2017-09-01T20:27:52Z

Oops, broke the test case; now fixed. Also fixed explanatory message per Carol.

Making sure this still passes: @dotnet-bot test Windows_NT x64 corefx_baseline

JosephTremoulet · 2017-09-01T20:31:39Z

+    // Unless they are invariant values, we need to evaluate them both
+    // to temps at those points to safely transmit the values here.
+    //
+    // Also we need ot use the flag twice, so we need two trees for it.


typo: "ot" -> "to"

Found one other typo; will fix them both.

jkotas · 2017-09-01T21:38:14Z

The type equivalence implementation needs to be more careful about inexact types in shared generic code. The CoreCLR type system is not capable of capturing the exact type information for shared generic code (ie it does not have equivalent of RuntimeDeterminedType from CoreRT type system).

Compile the following with /o-, exception will be thrown as expected.
Compile the following with /o+, no exception will be thrown because of the optimizations will consider the types to be identical even though they are actually not.

using System;
using System.Runtime.CompilerServices;

class MyG<T,U> {

public enum A {
   X = 1
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void foo() {
    var a = MyG<object,U>.A.X;
    a.HasFlag(MyG<T,string>.A.X);
}

}

class My {

static void Main() {
    MyG<My,My>.foo();
}

}

It may be nice to plumb this such that these optimization can kick in shared generic code when the EE side has exact types in shared generic code.

AndyAyersMS · 2017-09-01T21:47:31Z

@jkotas thanks for pointing this out. Think I know the right way to fix it...

jkotas · 2017-09-01T22:23:55Z

                }
            }
-            else if (pMT->HasSameTypeDefAs(MscorlibBinder::GetClass(CLASS__READONLY_SPAN)))
+            else if (pMT->HasSameTypeDefAs(MscorlibBinder::GetClass(CLASS__ENUM)))


The preferred way to define non-generic intrinsincs is to make them FCall and use FCIntrinsic to get them ID assigned in vm\ecalllist.h. The advantage of it is that the intrinsic ID is cached and the scheme scales well with the number of intrinsics.

For Enum.HasFlag, it would require moving the bit of error handling from C# to C++ so that the main entrypoint is FCall.

So basically move the checking in HasFlag down into InternalHasFlag, then remove the former and rename the latter as HasFlag?

Looked into this some -- it is not obvious how to create the proper exception with formatted text for the class equivalence check. Any suggestions?

jkotas · 2017-09-01T22:25:04Z

Otherwise LGTM.

AndyAyersMS · 2017-09-01T22:48:25Z

Looks like it needs some fixes for non-default underlying types too.

AndyAyersMS · 2017-09-01T23:16:40Z

@jkotas what is the explanation for the R4/R8 support in Enum.cs, for instance in GetValue ?

jkotas · 2017-09-02T15:20:58Z

dotnet/corefx#13981 (comment)

It may be nice to add comment about this at the top of Enum.cs. It is frequently asked question.

AndyAyersMS · 2017-09-05T15:06:48Z

Current thinking is to revise how Enum.HasFlag is implemented as a separate prerequisite PR, then come back to this (the jit side stuff is largely done).

@jkotas any advice on how to create the properly formatted type mismatch exception on the VM side? It seems to me like it will take a fair amount of extra plumbing...

jkotas · 2017-09-05T15:53:10Z

create the properly formatted type mismatch exception on the VM side?

I think that the easiest way to do it is to call back to managed code to throw the exception. MetadataImport.ThrowError is good example.

Maybe we should use this as an opportunity to create a protocol that allows arbitrary (non-FCall) CoreLib methods to be recognized as JIT intrinsic. Such scheme will be needed for SIMD intrinsics that are being worked on as well.

AndyAyersMS · 2017-09-05T18:46:56Z

Here's a fork with my take on a fully native Enum.HasFlag that calls back for the type mismatch exception case.

Crafting a more flexible mechanism for determining which methods can be jit intrinsics sounds like a good idea, though I am not clear on what all would be required.

jkotas · 2017-09-05T19:47:40Z

Crafting a more flexible mechanism for determining which methods can be jit intrinsics sounds like a good idea, though I am not clear on what all would be required.

Mark methods that are potential JIT intrinsics with [Intrinsic] attribute
Cache the intrinsic flag on MethodDesc
Fast check on JIT/EE interface to tell whether method is a potential JIT intrinsic (e.g. it can be a flag returned by getMethodAttribs)
If it is potential intrinsic, the JIT can ask for type name and method name and switch based on that. It is similar to what is done for Vector intrinsics today. Notice that this allows adding/removing JIT intrinsics without changing JIT/EE interface.

AndyAyersMS · 2017-09-05T23:19:04Z

@jkotas here's a first cut at the new style intrinsic support.

Let me know if you have a better name for these new kind of intrinsics and/or there is a better place to check for the attribute.

If this looks plausible I'll put it up for PR.

jkotas · 2017-09-05T23:42:35Z

Yes, it looks plausible - I have left a few comments on the commit.

jkotas · 2017-09-05T23:44:58Z

Let me know if you have a better name for these new kind of intrinsics

I would expect that we will migrate all JIT intrinsics over to this scheme over time, so I would not worry about the name too much.

Check for calls to `Enum.HasFlag` using the new named intrinsic support introduced in dotnet#13815. Implement a simple recognizer for these named intrinsics (currently just recognizing `Enum.HasFlag`). When the call is recognized, optimize if both operands are boxes with compatible types and both boxes can be removed. The optimization changes the call to a simple and/compare tree on the underlying enum values. To accomplish this, generalize the behavior of `gtTryRemoveBoxUpstreamEffects` to add a "trial removal" mode and to optionally suppress narrowing of the copy source. Invoke the optimization during importation (which will catch most cases) and again during morph (to get the post-inline cases). Added test cases. Suprisingly there were almost no uses of HasFlag in the current CoreCLR test suite. Closes #5626.

AndyAyersMS · 2017-09-07T23:03:11Z

Reworked this now that the new intrinsic support is in place.

Most of the code is as before, but the recognition bit is different. I put in some toeholds for a more general classification framework for the new intrinsics; eventually we can flesh this out with more information as we need it.

@JosephTremoulet PTAL
cc @dotnet/jit-contrib

AndyAyersMS · 2017-09-08T07:13:27Z

@dotnet-bot retest Ubuntu arm64 Cross Debug Build

JosephTremoulet · 2017-09-08T14:30:35Z

-//
-//    False if the upstream effects could not be removed.
+//    A tree representing the original value to box, if removal
+//    is successful (but see note). nullptr if removal fails.


FWIW, after reading this and the note I was still not sure what the return value is when BR_DONT_REMOVE gets passed... something like successful/possible instead of just successful here, or an explicit Still returns the source tree if removal is legal in the note for BR_DONT_REMOVE, would maybe have helped.

Will do the former.

JosephTremoulet

Looks good, just some very minor observations.

JosephTremoulet · 2017-09-08T14:33:03Z

+    }
+
+    // A boxed flagOp should have exact type and non-null instance
+    assert(isExactFlag && isNonNullFlag);


Nit: I always prefer assert(x); assert(y); over assert(x && y), so you know which failed when one fails...

JosephTremoulet · 2017-09-08T14:35:52Z

+    bool                 isNonNullFlag = false;
+    CORINFO_CLASS_HANDLE flagHnd       = gtGetClassHandle(flagOp, &isExactFlag, &isNonNullFlag);
+
+    if (flagHnd == nullptr)


You don't have the CORINFO_FLG_SHAREDINST check for the flag operand. I presume it couldn't equal thisHnd if it were shared... could consider adding a comment, or deferring the shared inst check until after the handles are checked for equality, just for readability.

Deferring the check.

AndyAyersMS · 2017-09-11T15:49:58Z

OSX test failure is unrelated: faililng pal test

..........................Expected thread to wait for 4000 ms (and not be interrupted).
Thread waited for 3999 ms! (Acceptable delta: 300)


FAILED: threading/WaitForSingleObject/WFSOExSemaphoreTest/paltest_waitforsingleobject_wfsoexsemaphoretest. Exit code: 1

AndyAyersMS · 2017-09-11T15:50:19Z

@dotnet-bot retest OSX10.12 x64 Checked Build and Test

AndyAyersMS · 2017-09-11T17:45:42Z

Tizen time out is worrisome, but the same changes passed before. Am going to retry.

@dotnet-bot retest Tizen armel Cross Debug Build

dnfclas added the cla-already-signed label Sep 1, 2017

JosephTremoulet approved these changes Sep 1, 2017

View reviewed changes

mikedn reviewed Sep 1, 2017

View reviewed changes

CarolEidt approved these changes Sep 1, 2017

View reviewed changes

JosephTremoulet reviewed Sep 1, 2017

View reviewed changes

jkotas reviewed Sep 1, 2017

View reviewed changes

karelz added the area-CodeGen label Sep 6, 2017

karelz assigned AndyAyersMS Sep 6, 2017

AndyAyersMS force-pushed the FixEnumFlagsCompare branch from e3e7322 to 9d0479e Compare September 7, 2017 22:59

JosephTremoulet reviewed Sep 8, 2017

View reviewed changes

JosephTremoulet approved these changes Sep 8, 2017

View reviewed changes

AndyAyersMS mentioned this pull request Sep 8, 2017

JIT: optimize case where box feeds GetType #13710

Merged

incorporate review feedback

79f9cf3

AndyAyersMS merged commit e764b4d into dotnet:master Sep 11, 2017

AndyAyersMS deleted the FixEnumFlagsCompare branch September 11, 2017 18:12

justinvp mentioned this pull request Sep 22, 2017

Optimize trivial cases of Enum.HasFlag calls dotnet/roslyn#12810

Closed

ScottKaye mentioned this pull request Nov 28, 2017

Is RCS1096 as useful anymore? dotnet/roslynator#212

Closed

benaadams mentioned this pull request Jul 27, 2018

Implement HTTP/2 input flow control aspnet/KestrelHttpServer#2740

Merged

YohDeadfall mentioned this pull request Jan 18, 2019

Change coordinate handling logic NetTopologySuite/NetTopologySuite.IO.PostGis#9

Merged

AndyAyersMS mentioned this pull request Jan 31, 2020

Add more flexible method for specifying jit instrinsics dotnet/runtime#8882

Closed

xtqqczze mentioned this pull request May 20, 2020

Suggestion: Call 'HasFlag' instead of using a bitwise operation dotnet/roslynator#679

Closed

sungam3r mentioned this pull request May 23, 2020

MultiChannelClientCommandDispatcher EasyNetQ/EasyNetQ#1059

Merged

MichalStrehovsky mentioned this pull request May 28, 2020

Support DynamicDependencyAttribute dotnet/linker#1215

Merged

halter73 mentioned this pull request Aug 7, 2020

Use HasFlag in ForwardedheadersMiddleware dotnet/aspnetcore#24660

Merged

mocsharp mentioned this pull request Jan 7, 2021

DICOMweb client - STOW APIs NVIDIA/clara-dicom-adapter#32

Merged

8 tasks

PaulusParssinen mentioned this pull request Jan 25, 2025

Switching Enum.HasFlag calls with bitwise operations microsoft/garnet#953

Merged

Conversation

AndyAyersMS commented Sep 1, 2017

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

JosephTremoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 1, 2017

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

AndyAyersMS commented Sep 1, 2017

Uh oh!

jkotas commented Sep 2, 2017

Uh oh!

AndyAyersMS commented Sep 5, 2017

Uh oh!

jkotas commented Sep 5, 2017

Uh oh!

AndyAyersMS commented Sep 5, 2017

Uh oh!

jkotas commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Sep 5, 2017

jkotas commented Sep 1, 2017 •

edited

Loading

jkotas commented Sep 5, 2017 •

edited

Loading