From 9d0479e92434019faae47579e613d0cbaf4f1876 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 7 Sep 2017 15:13:12 -0700 Subject: [PATCH 1/2] Jit: optimize Enum.HasFlag Check for calls to `Enum.HasFlag` using the new named intrinsic support introduced in #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. --- src/jit/compiler.h | 17 +- src/jit/gentree.cpp | 223 +++++++++++++++++++++++--- src/jit/importer.cpp | 105 +++++++++--- src/jit/morph.cpp | 22 ++- src/jit/namedintrinsiclist.h | 16 ++ tests/src/JIT/opt/Enum/hasflag.cs | 169 +++++++++++++++++++ tests/src/JIT/opt/Enum/hasflag.csproj | 39 +++++ tests/src/JIT/opt/Enum/shared.cs | 41 +++++ tests/src/JIT/opt/Enum/shared.csproj | 39 +++++ 9 files changed, 626 insertions(+), 45 deletions(-) create mode 100644 src/jit/namedintrinsiclist.h create mode 100644 tests/src/JIT/opt/Enum/hasflag.cs create mode 100644 tests/src/JIT/opt/Enum/hasflag.csproj create mode 100644 tests/src/JIT/opt/Enum/shared.cs create mode 100644 tests/src/JIT/opt/Enum/shared.csproj diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 01997f1665c5..c2ecb0dbae6d 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -45,6 +45,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX #include "valuenum.h" #include "reglist.h" #include "jittelemetry.h" +#include "namedintrinsiclist.h" #ifdef LATE_DISASM #include "disasm.h" #endif @@ -2237,7 +2238,17 @@ class Compiler gtFoldExprConst(GenTreePtr tree); GenTreePtr gtFoldExprSpecial(GenTreePtr tree); GenTreePtr gtFoldExprCompare(GenTreePtr tree); - bool gtTryRemoveBoxUpstreamEffects(GenTreePtr tree); + + // Options to control behavior of gtTryRemoveBoxUpstreamEffects + enum BoxRemovalOptions + { + BR_REMOVE_AND_NARROW, // remove effects and minimize remaining work + BR_REMOVE_BUT_NOT_NARROW, // remove effects but leave box copy source full size + BR_DONT_REMOVE // just check if removal is possible + }; + + GenTree* gtTryRemoveBoxUpstreamEffects(GenTree* tree, BoxRemovalOptions options = BR_REMOVE_AND_NARROW); + GenTree* gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp); //------------------------------------------------------------------------- // Get the handle, if any. @@ -2981,7 +2992,9 @@ class Compiler bool readonlyCall, bool tailCall, bool isJitIntrinsic, - CorInfoIntrinsics* pIntrinsicID); + CorInfoIntrinsics* pIntrinsicID, + bool* isSpecialIntrinsic = nullptr); + NamedIntrinsic lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method); GenTreePtr impArrayAccessIntrinsic(CORINFO_CLASS_HANDLE clsHnd, CORINFO_SIG_INFO* sig, int memberRef, diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index 8d7a950f3957..2547df047973 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -12206,7 +12206,8 @@ GenTreePtr Compiler::gtFoldExprSpecial(GenTreePtr tree) assert(!gtTreeHasSideEffects(op->gtBox.gtOp.gtOp1, GTF_SIDE_EFFECT)); // See if we can optimize away the box and related statements. - bool didOptimize = gtTryRemoveBoxUpstreamEffects(op); + GenTree* boxSourceTree = gtTryRemoveBoxUpstreamEffects(op); + bool didOptimize = (boxSourceTree != nullptr); // If optimization succeeded, remove the box. if (didOptimize) @@ -12449,18 +12450,30 @@ GenTreePtr Compiler::gtFoldExprSpecial(GenTreePtr tree) // the copy. // // Arguments: -// op -- the box node to optimize +// op - the box node to optimize +// options - controls whether and how trees are modified +// (see notes) // // Return Value: -// True if the upstream effects were removed. Note parts of the -// copy tree may remain, if the copy source had side effects. -// -// 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. // // Notes: // Value typed box gets special treatment because it has associated // side effects that can be removed if the box result is not used. // +// By default (options == BR_REMOVE_AND_NARROW) this method will +// try and remove unnecessary trees and will try and reduce remaning +// operations to the minimal set, possibly narrowing the width of +// loads from the box source if it is a struct. +// +// To perform a trial removal, pass BR_DONT_REMOVE. This can be +// useful to determine if this optimization should only be +// performed if some other conditions hold true. +// +// To remove but not alter the access to the box source, pass +// BR_REMOVE_BUT_NOT_NARROW. +// // If removal fails, is is possible that a subsequent pass may be // able to optimize. Blocking side effects may now be minimized // (null or bounds checks might have been removed) or might be @@ -12468,21 +12481,22 @@ GenTreePtr Compiler::gtFoldExprSpecial(GenTreePtr tree) // return expression). So the box is perhaps best left as is to // help trigger this re-examination. -bool Compiler::gtTryRemoveBoxUpstreamEffects(GenTreePtr op) +GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions options) { JITDUMP("gtTryRemoveBoxUpstreamEffects called for [%06u]\n", dspTreeID(op)); assert(op->IsBoxedValue()); // grab related parts for the optimization - GenTreePtr asgStmt = op->gtBox.gtAsgStmtWhenInlinedBoxValue; + GenTree* asgStmt = op->gtBox.gtAsgStmtWhenInlinedBoxValue; assert(asgStmt->gtOper == GT_STMT); - GenTreePtr copyStmt = op->gtBox.gtCopyStmtWhenInlinedBoxValue; + GenTree* copyStmt = op->gtBox.gtCopyStmtWhenInlinedBoxValue; assert(copyStmt->gtOper == GT_STMT); #ifdef DEBUG if (verbose) { - printf("\nAttempting to remove side effects of BOX (valuetype)\n"); + printf("\n%s to remove side effects of BOX (valuetype)\n", + options == BR_DONT_REMOVE ? "Checking if it is possible" : "Attempting"); gtDispTree(op); printf("\nWith assign\n"); gtDispTree(asgStmt); @@ -12492,15 +12506,15 @@ bool Compiler::gtTryRemoveBoxUpstreamEffects(GenTreePtr op) #endif // If we don't recognize the form of the assign, bail. - GenTreePtr asg = asgStmt->gtStmt.gtStmtExpr; + GenTree* asg = asgStmt->gtStmt.gtStmtExpr; if (asg->gtOper != GT_ASG) { JITDUMP(" bailing; unexpected assignment op %s\n", GenTree::OpName(asg->gtOper)); - return false; + return nullptr; } // If we don't recognize the form of the copy, bail. - GenTreePtr copy = copyStmt->gtStmt.gtStmtExpr; + GenTree* copy = copyStmt->gtStmt.gtStmtExpr; if (copy->gtOper != GT_ASG) { // GT_RET_EXPR is a tolerable temporary failure. @@ -12518,18 +12532,18 @@ bool Compiler::gtTryRemoveBoxUpstreamEffects(GenTreePtr op) // looking for. JITDUMP(" bailing; unexpected copy op %s\n", GenTree::OpName(copy->gtOper)); } - return false; + return nullptr; } // If the copy is a struct copy, make sure we know how to isolate // any source side effects. - GenTreePtr copySrc = copy->gtOp.gtOp2; + GenTree* copySrc = copy->gtOp.gtOp2; // If the copy source is from a pending inline, wait for it to resolve. if (copySrc->gtOper == GT_RET_EXPR) { JITDUMP(" bailing; must wait for replacement of copy source %s\n", GenTree::OpName(copySrc->gtOper)); - return false; + return nullptr; } bool hasSrcSideEffect = false; @@ -12548,12 +12562,18 @@ bool Compiler::gtTryRemoveBoxUpstreamEffects(GenTreePtr op) // We don't know how to handle other cases, yet. JITDUMP(" bailing; unexpected copy source struct op with side effect %s\n", GenTree::OpName(copySrc->gtOper)); - return false; + return nullptr; } } } - // Proceed with the optimization + // If this was a trial removal, we're done. + if (options == BR_DONT_REMOVE) + { + return copySrc; + } + + // Otherwise, proceed with the optimization. // // Change the assignment expression to a NOP. JITDUMP("\nBashing NEWOBJ [%06u] to NOP\n", dspTreeID(asg)); @@ -12585,10 +12605,18 @@ bool Compiler::gtTryRemoveBoxUpstreamEffects(GenTreePtr op) // source struct; there's no need to read the // entire thing, and no place to put it. assert(copySrc->gtOper == GT_OBJ || copySrc->gtOper == GT_IND || copySrc->gtOper == GT_FIELD); - copySrc->ChangeOper(GT_IND); - copySrc->gtType = TYP_BYTE; copyStmt->gtStmt.gtStmtExpr = copySrc; - JITDUMP(" to read first byte of struct via modified [%06u]\n", dspTreeID(copySrc)); + + if (options == BR_REMOVE_AND_NARROW) + { + JITDUMP(" to read first byte of struct via modified [%06u]\n", dspTreeID(copySrc)); + copySrc->ChangeOper(GT_IND); + copySrc->gtType = TYP_BYTE; + } + else + { + JITDUMP(" to read entire struct via modified [%06u]\n", dspTreeID(copySrc)); + } } if (fgStmtListThreaded) @@ -12597,8 +12625,157 @@ bool Compiler::gtTryRemoveBoxUpstreamEffects(GenTreePtr op) fgSetStmtSeq(copyStmt); } - // Box effects were successfully optimized - return true; + // Box effects were successfully optimized. + return copySrc; +} + +//------------------------------------------------------------------------ +// gtOptimizeEnumHasFlag: given the operands for a call to Enum.HasFlag, +// try and optimize the call to a simple and/compare tree. +// +// Arguments: +// thisOp - first argument to the call +// flagOp - second argument to the call +// +// Return Value: +// A new cmp/amd tree if successful. nullptr on failure. +// +// Notes: +// If successful, may allocate new temps and modify connected +// statements. + +GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp) +{ + JITDUMP("Considering optimizing call to Enum.HasFlag....\n"); + + // Operands must be boxes + if (!thisOp->IsBoxedValue() || !flagOp->IsBoxedValue()) + { + JITDUMP("bailing, need both inputs to be BOXes\n"); + return nullptr; + } + + // Operands must have same type + bool isExactThis = false; + bool isNonNullThis = false; + CORINFO_CLASS_HANDLE thisHnd = gtGetClassHandle(thisOp, &isExactThis, &isNonNullThis); + + if (thisHnd == nullptr) + { + JITDUMP("bailing, can't find type for 'this' operand\n"); + return nullptr; + } + + // If we have a shared type instance we can't safely check type + // equality, so bail. + DWORD classAttribs = info.compCompHnd->getClassAttribs(thisHnd); + if (classAttribs & CORINFO_FLG_SHAREDINST) + { + JITDUMP("bailing, have shared instance type for 'this' operand\n"); + return nullptr; + } + + // A boxed thisOp should have exact type and non-null instance + assert(isExactThis && isNonNullThis); + + bool isExactFlag = false; + bool isNonNullFlag = false; + CORINFO_CLASS_HANDLE flagHnd = gtGetClassHandle(flagOp, &isExactFlag, &isNonNullFlag); + + if (flagHnd == nullptr) + { + JITDUMP("bailing, can't find type for 'flag' operand\n"); + return nullptr; + } + + // A boxed flagOp should have exact type and non-null instance + assert(isExactFlag && isNonNullFlag); + + if (flagHnd != thisHnd) + { + JITDUMP("bailing, operand types differ\n"); + return nullptr; + } + + // Simulate removing the box for thisOP. We need to know that it can + // be safely removed before we can optimize. + GenTree* thisVal = gtTryRemoveBoxUpstreamEffects(thisOp, BR_DONT_REMOVE); + if (thisVal == nullptr) + { + // Note we may fail here if the this operand comes from + // a call. We should be able to retry this post-inlining. + JITDUMP("bailing, can't undo box of 'this' operand\n"); + return nullptr; + } + + GenTree* flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_REMOVE_BUT_NOT_NARROW); + if (flagVal == nullptr) + { + // Note we may fail here if the flag operand comes from + // a call. We should be able to retry this post-inlining. + JITDUMP("bailing, can't undo box of 'flag' operand\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. + thisVal = gtTryRemoveBoxUpstreamEffects(thisOp, BR_REMOVE_BUT_NOT_NARROW); + + // Our trial removal above should guarantee successful removal here. + assert(thisVal != nullptr); + + // We should have a consistent view of the type + var_types type = thisVal->TypeGet(); + assert(type == flagVal->TypeGet()); + + // The thisVal and flagVal trees come from earlier statements. + // + // 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 to use the flag twice, so we need two trees for it. + GenTree* thisValOpt = nullptr; + GenTree* flagValOpt = nullptr; + GenTree* flagValOptCopy = nullptr; + + if (thisVal->IsIntegralConst()) + { + thisValOpt = gtClone(thisVal); + } + else + { + const unsigned thisTmp = lvaGrabTemp(true DEBUGARG("Enum:HasFlag this temp")); + GenTree* thisAsg = gtNewTempAssign(thisTmp, thisVal); + GenTree* thisAsgStmt = thisOp->AsBox()->gtCopyStmtWhenInlinedBoxValue; + thisAsgStmt->gtStmt.gtStmtExpr = thisAsg; + thisValOpt = gtNewLclvNode(thisTmp, type); + } + + if (flagVal->IsIntegralConst()) + { + flagValOpt = gtClone(flagVal); + flagValOptCopy = gtClone(flagVal); + } + else + { + const unsigned flagTmp = lvaGrabTemp(true DEBUGARG("Enum:HasFlag flag temp")); + GenTree* flagAsg = gtNewTempAssign(flagTmp, flagVal); + GenTree* flagAsgStmt = flagOp->AsBox()->gtCopyStmtWhenInlinedBoxValue; + flagAsgStmt->gtStmt.gtStmtExpr = flagAsg; + flagValOpt = gtNewLclvNode(flagTmp, type); + flagValOptCopy = gtNewLclvNode(flagTmp, type); + } + + // Turn the call into (thisValTmp & flagTmp) == flagTmp. + GenTree* andTree = gtNewOperNode(GT_AND, type, thisValOpt, flagValOpt); + GenTree* cmpTree = gtNewOperNode(GT_EQ, TYP_INT, andTree, flagValOptCopy); + + JITDUMP("Optimized call to Enum.HasFlag\n"); + + return cmpTree; } /***************************************************************************** diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index dab2ee30a3a5..42bb84d93a34 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -3282,9 +3282,11 @@ GenTreePtr Compiler::impIntrinsic(GenTreePtr newobjThis, bool readonlyCall, bool tailCall, bool isJitIntrinsic, - CorInfoIntrinsics* pIntrinsicID) + CorInfoIntrinsics* pIntrinsicID, + bool* isSpecialIntrinsic) { bool mustExpand = false; + bool isSpecial = false; CorInfoIntrinsics intrinsicID = info.compCompHnd->getIntrinsicID(method, &mustExpand); *pIntrinsicID = intrinsicID; @@ -3617,6 +3619,9 @@ GenTreePtr Compiler::impIntrinsic(GenTreePtr newobjThis, // CORINFO_INTRINSIC_Object_GetType intrinsic can throw NullReferenceException. op1->gtFlags |= (GTF_CALL | GTF_EXCEPT); retNode = op1; + + // Might be optimizable during morph + isSpecial = true; break; #endif // Implement ByReference Ctor. This wraps the assignment of the ref into a byref-like field @@ -3759,33 +3764,55 @@ GenTreePtr Compiler::impIntrinsic(GenTreePtr newobjThis, break; } + case CORINFO_INTRINSIC_TypeEQ: + case CORINFO_INTRINSIC_TypeNEQ: + case CORINFO_INTRINSIC_GetCurrentManagedThread: + case CORINFO_INTRINSIC_GetManagedThreadId: + { + // Retry optimizing these during morph + isSpecial = true; + break; + } + default: /* Unknown intrinsic */ break; } -#ifdef DEBUG - // Sample code showing how to use the new intrinsic mechansim. + // Look for new-style jit intrinsics by name if (isJitIntrinsic) { assert(retNode == nullptr); - const char* className = nullptr; - const char* namespaceName = nullptr; - const char* methodName = info.compCompHnd->getMethodNameFromMetadata(method, &className, &namespaceName); + const NamedIntrinsic ni = lookupNamedIntrinsic(method); - if ((namespaceName != nullptr) && strcmp(namespaceName, "System") == 0) + switch (ni) { - if ((className != nullptr) && strcmp(className, "Enum") == 0) + case NI_Enum_HasFlag: { - if ((methodName != nullptr) && strcmp(methodName, "HasFlag") == 0) + GenTree* thisOp = impStackTop(1).val; + GenTree* flagOp = impStackTop(0).val; + GenTree* optTree = gtOptimizeEnumHasFlag(thisOp, flagOp); + + if (optTree != nullptr) + { + // Optimization successful. Pop the stack for real. + impPopStack(); + impPopStack(); + retNode = optTree; + } + else { - // Todo: plug in the intrinsic expansion - JITDUMP("Found Intrinsic call to Enum.HasFlag\n"); + // Retry optimizing this during morph. + isSpecial = true; } + + break; } + + default: + break; } } -#endif if (mustExpand) { @@ -3795,9 +3822,52 @@ GenTreePtr Compiler::impIntrinsic(GenTreePtr newobjThis, } } + // Optionally report if this intrinsic is special + // (that is, potentially re-optimizable during morph). + if (isSpecialIntrinsic != nullptr) + { + *isSpecialIntrinsic = isSpecial; + } + return retNode; } +//------------------------------------------------------------------------ +// lookupNamedIntrinsic: map method to jit named intrinsic value +// +// Arguments: +// method -- method handle for method +// +// Return Value: +// Id for the named intrinsic, or Illegal if none. +// +// Notes: +// method should have CORINFO_FLG_JIT_INTRINSIC set in its attributes, +// otherwise it is not a named jit intrinsic. +// + +NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method) +{ + NamedIntrinsic result = NI_Illegal; + + const char* className = nullptr; + const char* namespaceName = nullptr; + const char* methodName = info.compCompHnd->getMethodNameFromMetadata(method, &className, &namespaceName); + + if ((namespaceName != nullptr) && strcmp(namespaceName, "System") == 0) + { + if ((className != nullptr) && strcmp(className, "Enum") == 0) + { + if ((methodName != nullptr) && strcmp(methodName, "HasFlag") == 0) + { + result = NI_Enum_HasFlag; + } + } + } + + return result; +} + /*****************************************************************************/ GenTreePtr Compiler::impArrayAccessIntrinsic( @@ -6808,12 +6878,13 @@ var_types Compiler::impImportCall(OPCODE opcode, #endif // DEBUG // Factor this into getCallInfo - const bool isIntrinsic = (mflags & CORINFO_FLG_INTRINSIC) != 0; - const bool isJitIntrinsic = (mflags & CORINFO_FLG_JIT_INTRINSIC) != 0; + const bool isIntrinsic = (mflags & CORINFO_FLG_INTRINSIC) != 0; + const bool isJitIntrinsic = (mflags & CORINFO_FLG_JIT_INTRINSIC) != 0; + bool isSpecialIntrinsic = false; if ((isIntrinsic || isJitIntrinsic) && !pConstrainedResolvedToken) { call = impIntrinsic(newobjThis, clsHnd, methHnd, sig, pResolvedToken->token, readonlyCall, - (canTailCall && (tailCall != 0)), isJitIntrinsic, &intrinsicID); + (canTailCall && (tailCall != 0)), isJitIntrinsic, &intrinsicID, &isSpecialIntrinsic); if (compIsForInlining() && compInlineResult->IsFailure()) { @@ -7129,9 +7200,7 @@ var_types Compiler::impImportCall(OPCODE opcode, } // Mark call if it's one of the ones we will maybe treat as an intrinsic - if (intrinsicID == CORINFO_INTRINSIC_Object_GetType || intrinsicID == CORINFO_INTRINSIC_TypeEQ || - intrinsicID == CORINFO_INTRINSIC_TypeNEQ || intrinsicID == CORINFO_INTRINSIC_GetCurrentManagedThread || - intrinsicID == CORINFO_INTRINSIC_GetManagedThreadId) + if (isSpecialIntrinsic) { call->gtCall.gtCallMoreFlags |= GTF_CALL_M_SPECIAL_INTRINSIC; } diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index ddaf278cc1af..ea2a3a7c8940 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -8839,12 +8839,30 @@ GenTreePtr Compiler::fgMorphCall(GenTreeCall* call) compCurBB->bbFlags |= BBF_GC_SAFE_POINT; } - // Morph Type.op_Equality and Type.op_Inequality - // We need to do this before the arguments are morphed + // Morph Type.op_Equality, Type.op_Inequality, and Enum.HasFlag + // + // We need to do these before the arguments are morphed if ((call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC)) { CorInfoIntrinsics methodID = info.compCompHnd->getIntrinsicID(call->gtCallMethHnd); + if (methodID == CORINFO_INTRINSIC_Illegal) + { + // Check for a new-style jit intrinsic. + const NamedIntrinsic ni = lookupNamedIntrinsic(call->gtCallMethHnd); + + if (ni == NI_Enum_HasFlag) + { + GenTree* thisOp = call->gtCallObjp; + GenTree* flagOp = call->gtCallArgs->gtOp.gtOp1; + GenTree* optTree = gtOptimizeEnumHasFlag(thisOp, flagOp); + if (optTree != nullptr) + { + return fgMorphTree(optTree); + } + } + } + genTreeOps simpleOp = GT_CALL; if (methodID == CORINFO_INTRINSIC_TypeEQ) { diff --git a/src/jit/namedintrinsiclist.h b/src/jit/namedintrinsiclist.h new file mode 100644 index 000000000000..cf81afc119da --- /dev/null +++ b/src/jit/namedintrinsiclist.h @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#ifndef _NAMEDINTRINSICLIST_H_ +#define _NAMEDINTRINSICLIST_H_ + +// Named jit intrinsics + +enum NamedIntrinsic +{ + NI_Illegal = 0, + NI_Enum_HasFlag = 1 +}; + +#endif // _NAMEDINTRINSICLIST_H_ diff --git a/tests/src/JIT/opt/Enum/hasflag.cs b/tests/src/JIT/opt/Enum/hasflag.cs new file mode 100644 index 000000000000..08b418b69497 --- /dev/null +++ b/tests/src/JIT/opt/Enum/hasflag.cs @@ -0,0 +1,169 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +// Some simple tests for the Enum.HasFlag optimization. +// +// All the calls to HasFlag below are now optimized to simple compares, +// except for the case in the try/catch. + +using System; + +enum E +{ + ZERO, + RED, + BLUE +} + +enum F : ulong +{ + ZERO, + RED = 0x800000000UL, + BLUE = 0x1000000000UL +} + +enum G : byte +{ + ZERO, + RED = 6, + BLUE = 9 +} + +struct EStruct +{ + public E e; +} + +class EClass +{ + public E e; +} + +class P +{ + static E[] ArrayOfE = { E.RED, E.BLUE }; + + static E StaticE = E.BLUE; + + static E GetE() + { + return E.RED; + } + + static E GetESideEffect() + { + E e = StaticE; + switch (e) + { + case E.RED: + StaticE = E.BLUE; + break; + case E.BLUE: + StaticE = E.RED; + break; + } + return e; + } + + public static bool ByrefE(ref E e1, E e2) + { + return e1.HasFlag(e2); + } + + public static bool ByrefF(ref F e1, F e2) + { + return e1.HasFlag(e2); + } + + public static bool ByrefG(ref G e1, G e2) + { + return e1.HasFlag(e2); + } + + public static int Main() + { + E e1 = E.RED; + E e2 = E.BLUE; + + EClass ec = new EClass(); + ec.e = E.RED; + + EStruct es = new EStruct(); + es.e = E.BLUE; + + bool true0 = E.RED.HasFlag(E.RED); + bool true1 = e1.HasFlag(GetE()); + bool true2 = ArrayOfE[0].HasFlag(E.RED); + bool true3 = StaticE.HasFlag(E.BLUE); + bool true4 = ec.e.HasFlag(e1); + bool true5 = e2.HasFlag(es.e); + bool true6 = e1.HasFlag(E.ZERO); + bool true7 = e2.HasFlag(E.ZERO); + + bool false0 = E.BLUE.HasFlag(E.RED); + bool false1 = e1.HasFlag(e2); + bool false2 = GetE().HasFlag(e2); + bool false3 = E.RED.HasFlag(ArrayOfE[1]); + bool false4 = E.BLUE.HasFlag(ec.e); + bool false5 = ByrefE(ref e1, StaticE); + + bool true8 = StaticE.HasFlag(GetESideEffect()); + bool false6 = GetESideEffect().HasFlag(StaticE); + + bool false7 = F.RED.HasFlag(F.BLUE); + bool false8 = G.RED.HasFlag(G.BLUE); + + F f1 = F.RED; + bool false9 = ByrefF(ref f1, F.BLUE); + + G g1 = G.RED; + bool true9 = ByrefG(ref g1, G.RED); + + 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 resultOk = true; + + int i = 0; + foreach (var b in trueResults) + { + i++; + if (!b) + { + Console.WriteLine("true{0} failed\n", i); + resultOk = false; + } + } + + i = 0; + foreach (var b in falseResults) + { + i++; + if (b) + { + Console.WriteLine("false{0} failed\n", i); + resultOk = false; + } + } + + // Optimization should bail on this case which causes an exception. + bool didThrow = false; + try + { + bool badFlag = E.RED.HasFlag((Enum) F.RED); + } + catch (ArgumentException) + { + didThrow = true; + } + + if (!didThrow) + { + Console.WriteLine("exception case failed\n"); + resultOk = false; + } + + return resultOk ? 100 : 0; + } +} diff --git a/tests/src/JIT/opt/Enum/hasflag.csproj b/tests/src/JIT/opt/Enum/hasflag.csproj new file mode 100644 index 000000000000..42e9f46eba09 --- /dev/null +++ b/tests/src/JIT/opt/Enum/hasflag.csproj @@ -0,0 +1,39 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT .0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + 1 + + + + + + + False + + + + PdbOnly + True + + + + + + + + + + \ No newline at end of file diff --git a/tests/src/JIT/opt/Enum/shared.cs b/tests/src/JIT/opt/Enum/shared.cs new file mode 100644 index 000000000000..f15361a05aa3 --- /dev/null +++ b/tests/src/JIT/opt/Enum/shared.cs @@ -0,0 +1,41 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +// Some simple tests for the Enum.HasFlag optimization. +// Verify the optimization avoids firing for shared types. + +using System; +using System.Runtime.CompilerServices; + +class MyG +{ + public enum A + { + X = 1 + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void foo() + { + var a = MyG.A.X; + a.HasFlag(MyG.A.X); + } +} + +class My +{ + public static int Main() + { + int result = 0; + try + { + MyG.foo(); + } + catch(ArgumentException) + { + result = 100; + } + return result; + } +} diff --git a/tests/src/JIT/opt/Enum/shared.csproj b/tests/src/JIT/opt/Enum/shared.csproj new file mode 100644 index 000000000000..343593f21fcc --- /dev/null +++ b/tests/src/JIT/opt/Enum/shared.csproj @@ -0,0 +1,39 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT .0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + 1 + + + + + + + False + + + + PdbOnly + True + + + + + + + + + + \ No newline at end of file From 79f9cf3aabfbd1fd83678f7df767c9eeebf72c4f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 11 Sep 2017 08:10:34 -0700 Subject: [PATCH 2/2] incorporate review feedback --- src/jit/gentree.cpp | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index 2547df047973..1190026f21aa 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -12456,7 +12456,7 @@ GenTreePtr Compiler::gtFoldExprSpecial(GenTreePtr tree) // // Return Value: // A tree representing the original value to box, if removal -// is successful (but see note). nullptr if removal fails. +// is successful/possible (but see note). nullptr if removal fails. // // Notes: // Value typed box gets special treatment because it has associated @@ -12666,17 +12666,9 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp) return nullptr; } - // If we have a shared type instance we can't safely check type - // equality, so bail. - DWORD classAttribs = info.compCompHnd->getClassAttribs(thisHnd); - if (classAttribs & CORINFO_FLG_SHAREDINST) - { - JITDUMP("bailing, have shared instance type for 'this' operand\n"); - return nullptr; - } - // A boxed thisOp should have exact type and non-null instance - assert(isExactThis && isNonNullThis); + assert(isExactThis); + assert(isNonNullThis); bool isExactFlag = false; bool isNonNullFlag = false; @@ -12689,7 +12681,8 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp) } // A boxed flagOp should have exact type and non-null instance - assert(isExactFlag && isNonNullFlag); + assert(isExactFlag); + assert(isNonNullFlag); if (flagHnd != thisHnd) { @@ -12697,6 +12690,15 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp) return nullptr; } + // If we have a shared type instance we can't safely check type + // equality, so bail. + DWORD classAttribs = info.compCompHnd->getClassAttribs(thisHnd); + if (classAttribs & CORINFO_FLG_SHAREDINST) + { + JITDUMP("bailing, have shared instance type\n"); + return nullptr; + } + // Simulate removing the box for thisOP. We need to know that it can // be safely removed before we can optimize. GenTree* thisVal = gtTryRemoveBoxUpstreamEffects(thisOp, BR_DONT_REMOVE);