Skip to content

Commit

Permalink
Creating comma temps differently for SubMulDiv morph (dotnet#69770)
Browse files Browse the repository at this point in the history 
* Creating comma temps differently for SubMulDiv morph

* Fixing some formatting. Added comments. Renamed fgMustMakeTemp to fgIsCloneableInvariantOrLocal

* Renaming

* Putting back in a check

* Formatting

* Adding comments

* Formatting
  • Loading branch information
@TIHan
TIHan committed Jun 1, 2022
1 parent 72a6f6a commit 5772d54
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 39 deletions.
8 changes: 8 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1651,6 +1651,12 @@ struct FuncInfoDsc
// that isn't shared between the main function body and funclets.
};

struct TempInfo
{
GenTree* asg;
GenTree* load;
};

#ifdef DEBUG
// XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
// We have the ability to mark source expressions with "Test Labels."
Expand Down Expand Up @@ -5490,6 +5496,8 @@ class Compiler
// Create a new temporary variable to hold the result of *ppTree,
// and transform the graph accordingly.
GenTree* fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
bool fgIsSafeToClone(GenTree* tree);
TempInfo fgMakeTemp(GenTree* rhs, CORINFO_CLASS_HANDLE structType = nullptr);
GenTree* fgMakeMultiUse(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);

// Recognize a bitwise rotation pattern and convert into a GT_ROL or a GT_ROR node.
Expand Down
171 changes: 132 additions & 39 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1817,6 +1817,72 @@ void CallArgs::SetNeedsTemp(CallArg* arg)
m_needsTemps = true;
}

//------------------------------------------------------------------------------
// fgIsSafeToClone: If the node is an unaliased local or constant,
// then it is safe to clone.
//
// Arguments:
// tree - The node to check if it is safe to clone.
//
// Return Value:
// True if the tree is cloneable. False if the tree is not cloneable.
//
// Notes:
// This is conservative as this will return False if the local's address
// is exposed.
//
bool Compiler::fgIsSafeToClone(GenTree* tree)
{
if (tree->IsInvariant())
{
return true;
}
else if (tree->IsLocal())
{
// Can't rely on GTF_GLOB_REF here.
//
if (!lvaGetDesc(tree->AsLclVarCommon())->IsAddressExposed())
{
return true;
}
}

return false;
}

//------------------------------------------------------------------------------
// fgMakeTemp: Make a temp variable with a right-hand side expression as the assignment.
//
// Arguments:
// rhs - The right-hand side expression.
//
// Return Value:
// 'TempInfo' data that contains the GT_ASG and GT_LCL_VAR nodes for assignment
// and variable load respectively.
//
TempInfo Compiler::fgMakeTemp(GenTree* rhs, CORINFO_CLASS_HANDLE structType /*= nullptr*/)
{
unsigned lclNum = lvaGrabTemp(true DEBUGARG("fgMakeTemp is creating a new local variable"));

if (varTypeIsStruct(rhs))
{
assert(structType != nullptr);
lvaSetStruct(lclNum, structType, false);
}

// If rhs->TypeGet() == TYP_STRUCT, gtNewTempAssign() will create a GT_COPYBLK tree.
// The type of GT_COPYBLK is TYP_VOID. Therefore, we should use rhs->TypeGet() for
// setting type of lcl vars created.
GenTree* asg = gtNewTempAssign(lclNum, rhs);
GenTree* load = new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, rhs->TypeGet(), lclNum);

TempInfo tempInfo{};
tempInfo.asg = asg;
tempInfo.load = load;

return tempInfo;
}

//------------------------------------------------------------------------------
// fgMakeMultiUse : If the node is an unaliased local or constant clone it,
// otherwise insert a comma form temp
Expand All @@ -1840,19 +1906,10 @@ GenTree* Compiler::fgMakeMultiUse(GenTree** pOp, CORINFO_CLASS_HANDLE structType
{
GenTree* const tree = *pOp;

if (tree->IsInvariant())
if (fgIsSafeToClone(tree))
{
return gtClone(tree);
}
else if (tree->IsLocal())
{
// Can't rely on GTF_GLOB_REF here.
//
if (!lvaGetDesc(tree->AsLclVarCommon())->IsAddressExposed())
{
return gtClone(tree);
}
}

return fgInsertCommaFormTemp(pOp, structType);
}
Expand All @@ -1875,26 +1932,13 @@ GenTree* Compiler::fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE
{
GenTree* subTree = *ppTree;

unsigned lclNum = lvaGrabTemp(true DEBUGARG("fgInsertCommaFormTemp is creating a new local variable"));
TempInfo tempInfo = fgMakeTemp(subTree, structType);
GenTree* asg = tempInfo.asg;
GenTree* load = tempInfo.load;

if (varTypeIsStruct(subTree))
{
assert(structType != nullptr);
lvaSetStruct(lclNum, structType, false);
}
*ppTree = gtNewOperNode(GT_COMMA, subTree->TypeGet(), asg, load);

// If subTree->TypeGet() == TYP_STRUCT, gtNewTempAssign() will create a GT_COPYBLK tree.
// The type of GT_COPYBLK is TYP_VOID. Therefore, we should use subTree->TypeGet() for
// setting type of lcl vars created.
GenTree* asg = gtNewTempAssign(lclNum, subTree);

GenTree* load = new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, subTree->TypeGet(), lclNum);

GenTree* comma = gtNewOperNode(GT_COMMA, subTree->TypeGet(), asg, load);

*ppTree = comma;

return new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, subTree->TypeGet(), lclNum);
return gtClone(load);
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -13814,6 +13858,28 @@ GenTree* Compiler::fgMorphMultiOp(GenTreeMultiOp* multiOp)
// division will be used, in that case this transform allows CSE to
// eliminate the redundant div from code like "x = a / 3; y = a % 3;".
//
// Before:
// * RETURN int
// \--* MOD int
// +--* MUL int
// | +--* LCL_VAR int V00 arg0
// | \--* LCL_VAR int V00 arg0
// \--* LCL_VAR int V01 arg1
// After:
// * RETURN int
// \--* COMMA int
// +--* ASG int
// | +--* LCL_VAR int V03 tmp1
// | \--* MUL int
// | +--* LCL_VAR int V00 arg0
// | \--* LCL_VAR int V00 arg0
// \--* SUB int
// +--* LCL_VAR int V03 tmp1
// \--* MUL int
// +--* DIV int
// | +--* LCL_VAR int V03 tmp1
// | \--* LCL_VAR int V01 arg1
// \--* LCL_VAR int V01 arg1
GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree)
{
JITDUMP("\nMorphing MOD/UMOD [%06u] to Sub/Mul/Div\n", dspTreeID(tree));
Expand All @@ -13831,24 +13897,51 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree)
noway_assert(!"Illegal gtOper in fgMorphModToSubMulDiv");
}

var_types type = tree->gtType;
GenTreeOp* const div = tree;

GenTree* const copyOfNumeratorValue = fgMakeMultiUse(&tree->gtOp1);
GenTree* const copyOfDenominatorValue = fgMakeMultiUse(&tree->gtOp2);
GenTree* const mul = gtNewOperNode(GT_MUL, type, tree, copyOfDenominatorValue);
GenTree* const sub = gtNewOperNode(GT_SUB, type, copyOfNumeratorValue, mul);
GenTree* dividend = div->gtGetOp1();
GenTree* divisor = div->gtGetOp2();

// Ensure "sub" does not evaluate "copyOfNumeratorValue" before it is defined by "mul".
//
sub->gtFlags |= GTF_REVERSE_OPS;
TempInfo tempInfos[2]{};
int tempInfoCount = 0;

if (!fgIsSafeToClone(dividend))
{
tempInfos[tempInfoCount] = fgMakeTemp(dividend);
dividend = tempInfos[tempInfoCount].load;
tempInfoCount++;
}

if (!fgIsSafeToClone(divisor))
{
tempInfos[tempInfoCount] = fgMakeTemp(divisor);
divisor = tempInfos[tempInfoCount].load;
tempInfoCount++;
}

var_types type = div->gtType;

div->gtOp1 = gtClone(dividend);
div->gtOp2 = gtClone(divisor);

GenTree* const mul = gtNewOperNode(GT_MUL, type, div, divisor);
GenTree* const sub = gtNewOperNode(GT_SUB, type, dividend, mul);

GenTree* result = sub;
// We loop backwards as it is easier to create new commas
// within one another for their sequence order.
for (int i = tempInfoCount - 1; i >= 0; i--)
{
result = gtNewOperNode(GT_COMMA, type, tempInfos[i].asg, result);
}

#ifdef DEBUG
sub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
#endif

tree->CheckDivideByConstOptimized(this);
div->CheckDivideByConstOptimized(this);

return sub;
return result;
}

//------------------------------------------------------------------------
Expand Down

0 comments on commit 5772d54

Please sign in to comment.