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

JIT: optimize Enum.HasFlag #13748

Merged
AndyAyersMS merged 2 commits into from
Sep 11, 2017
Merged

JIT: optimize Enum.HasFlag #13748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d0479e
Jit: optimize Enum.HasFlag
AndyAyersMS Sep 7, 2017
79f9cf3
incorporate review feedback
AndyAyersMS Sep 11, 2017
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
17 changes: 15 additions & 2 deletions src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#include "valuenum.h"
#include "reglist.h"
#include "jittelemetry.h"
#include "namedintrinsiclist.h"
#ifdef LATE_DISASM
#include "disasm.h"
#endif
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
225 changes: 202 additions & 23 deletions src/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -12449,40 +12450,53 @@ 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/possible (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
// better known (inline return placeholder updated with the actual
// 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;
Copy link
Copy Markdown

@JosephTremoulet JosephTremoulet Sep 1, 2017

Choose a reason for hiding this comment

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

Why GenTreePtr -> GenTree*?

Copy link
Copy Markdown
Member Author

@AndyAyersMS AndyAyersMS Sep 1, 2017

Choose a reason for hiding this comment

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

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

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);
Expand All @@ -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.
Expand All @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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)
Expand All @@ -12597,8 +12625,159 @@ 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;
}

// A boxed thisOp should have exact type and non-null instance
assert(isExactThis);
assert(isNonNullThis);

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

if (flagHnd == nullptr)
Copy link
Copy Markdown

@JosephTremoulet JosephTremoulet Sep 8, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@AndyAyersMS AndyAyersMS Sep 11, 2017

Choose a reason for hiding this comment

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

Deferring the check.

{
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);
assert(isNonNullFlag);

if (flagHnd != thisHnd)
{
JITDUMP("bailing, operand types differ\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\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;
}

/*****************************************************************************
Expand Down
Loading