Skip to content
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
22 changes: 18 additions & 4 deletions src/coreclr/src/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3236,7 +3236,8 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen
return nullptr;
}

AssertionDsc* curAssertion = optGetAssertion(index);
AssertionDsc* curAssertion = optGetAssertion(index);
bool assertionKindIsEqual = (curAssertion->assertionKind == OAK_EQUAL);

// Allow or not to reverse condition for OAK_NOT_EQUAL assertions.
bool allowReverse = true;
Expand All @@ -3251,7 +3252,7 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen
printf("\nVN relop based constant assertion prop in " FMT_BB ":\n", compCurBB->bbNum);
printf("Assertion index=#%02u: ", index);
printTreeID(op1);
printf(" %s ", (curAssertion->assertionKind == OAK_EQUAL) ? "==" : "!=");
printf(" %s ", assertionKindIsEqual ? "==" : "!=");
if (genActualType(op1->TypeGet()) == TYP_INT)
{
printf("%d\n", vnStore->ConstantValue<int>(vnCns));
Expand Down Expand Up @@ -3336,8 +3337,15 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen

op1->gtVNPair.SetBoth(vnCns); // Preserve the ValueNumPair, as ChangeOperConst/SetOper will clear it.

// Also set the value number on the relop.
if (curAssertion->assertionKind == OAK_EQUAL)
// set foldResult to either 0 or 1
bool foldResult = assertionKindIsEqual;
if (tree->gtOper == GT_NE)
{
foldResult = !foldResult;
}

// Set the value number on the relop to 1 (true) or 0 (false)
if (foldResult)
{
tree->gtVNPair.SetBoth(vnStore->VNOneForType(TYP_INT));
}
Expand Down Expand Up @@ -4947,6 +4955,12 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test)
//
Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, Statement* stmt, GenTree* tree)
{
// Don't perform const prop on expressions marked with GTF_DONT_CSE
if (!tree->CanCSE())
{
return WALK_CONTINUE;
}

// Don't propagate floating-point constants into a TYP_STRUCT LclVar
// This can occur for HFA return values (see hfa_sf3E_r.exe)
if (tree->TypeGet() == TYP_STRUCT)
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/src/jit/block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ void BasicBlock::dspFlags()
{
printf("jmp ");
}
if (bbFlags & BBF_HAS_CALL)
{
printf("hascall ");
}
if (bbFlags & BBF_GC_SAFE_POINT)
{
printf("gcsafe ");
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/src/jit/earlyprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,14 @@ GenTree* Compiler::optEarlyPropRewriteTree(GenTree* tree, LocalNumberToNullCheck
// actualValClone has small tree node size, it is safe to use CopyFrom here.
tree->ReplaceWith(actualValClone, this);

// Propagating a constant may create an opportunity to use a division by constant optimization
//
if ((tree->gtNext != nullptr) && tree->gtNext->OperIsBinary())
{
// We need to mark the parent divide/mod operation when this occurs
tree->gtNext->AsOp()->CheckDivideByConstOptimized(this);
}

#ifdef DEBUG
if (verbose)
{
Expand Down
205 changes: 197 additions & 8 deletions src/coreclr/src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -615,10 +615,6 @@ void GenTree::CopyReg(GenTree* from)
// GT_COPY/GT_RELOAD is considered having a reg if it
// has a reg assigned to any of its positions.
//
// Assumption:
// In order for this to work properly, gtClearReg must be called
// prior to setting the register value.
//
bool GenTree::gtHasReg() const
{
bool hasReg = false;
Expand Down Expand Up @@ -6674,6 +6670,173 @@ void GenTreeIntCon::FixupInitBlkValue(var_types asgType)
}
}

//----------------------------------------------------------------------------
// UsesDivideByConstOptimized:
// returns true if rationalize will use the division by constant
// optimization for this node.
//
// Arguments:
// this - a GenTreeOp node
// comp - the compiler instance
//
// Return Value:
// Return true iff the node is a GT_DIV,GT_UDIV, GT_MOD or GT_UMOD with
// an integer constant and we can perform the division operation using
// a reciprocal multiply or a shift operation.
//
bool GenTreeOp::UsesDivideByConstOptimized(Compiler* comp)
{
if (!comp->opts.OptimizationEnabled())
{
return false;
}

if (!OperIs(GT_DIV, GT_MOD, GT_UDIV, GT_UMOD))
{
return false;
}
#if defined(TARGET_ARM64)
if (OperIs(GT_MOD, GT_UMOD))
{
// MOD, UMOD not supported for ARM64
return false;
}
#endif // TARGET_ARM64

bool isSignedDivide = OperIs(GT_DIV, GT_MOD);
GenTree* dividend = gtGetOp1()->gtEffectiveVal(/*commaOnly*/ true);
GenTree* divisor = gtGetOp2()->gtEffectiveVal(/*commaOnly*/ true);

#if !defined(TARGET_64BIT)
if (dividend->OperIs(GT_LONG))
{
return false;
}
#endif

if (dividend->IsCnsIntOrI())
{
// We shouldn't see a divmod with constant operands here but if we do then it's likely
// because optimizations are disabled or it's a case that's supposed to throw an exception.
// Don't optimize this.
return false;
}

ssize_t divisorValue;
if (divisor->IsCnsIntOrI())
{
divisorValue = static_cast<ssize_t>(divisor->AsIntCon()->IconValue());
}
else
{
ValueNum vn = divisor->gtVNPair.GetLiberal();
if (comp->vnStore->IsVNConstant(vn))
{
divisorValue = comp->vnStore->CoercedConstantValue<ssize_t>(vn);
}
else
{
return false;
}
}

const var_types divType = TypeGet();

if (divisorValue == 0)
{
// x / 0 and x % 0 can't be optimized because they are required to throw an exception.
return false;
}
else if (isSignedDivide)
{
if (divisorValue == -1)
{
// x / -1 can't be optimized because INT_MIN / -1 is required to throw an exception.
return false;
}
else if (isPow2(divisorValue))
{
return true;
}
}
else // unsigned divide
{
if (divType == TYP_INT)
{
// Clear up the upper 32 bits of the value, they may be set to 1 because constants
// are treated as signed and stored in ssize_t which is 64 bit in size on 64 bit targets.
divisorValue &= UINT32_MAX;
}

size_t unsignedDivisorValue = (size_t)divisorValue;
if (isPow2(unsignedDivisorValue))
{
return true;
}
}

const bool isDiv = OperIs(GT_DIV, GT_UDIV);

if (isDiv)
{
if (isSignedDivide)
{
// If the divisor is the minimum representable integer value then the result is either 0 or 1
if ((divType == TYP_INT && divisorValue == INT_MIN) || (divType == TYP_LONG && divisorValue == INT64_MIN))
{
return true;
}
}
else
{
// If the divisor is greater or equal than 2^(N - 1) then the result is either 0 or 1
if (((divType == TYP_INT) && (divisorValue > (UINT32_MAX / 2))) ||
((divType == TYP_LONG) && (divisorValue > (UINT64_MAX / 2))))
{
return true;
}
}
}

// TODO-ARM-CQ: Currently there's no GT_MULHI for ARM32
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
if (!comp->opts.MinOpts() && ((divisorValue >= 3) || !isSignedDivide))
{
// All checks pass we can perform the division operation using a reciprocal multiply.
return true;
}
#endif

return false;
}

//------------------------------------------------------------------------
// CheckDivideByConstOptimized:
// Checks if we can use the division by constant optimization
// on this node
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: formatting.

Copy link
Contributor

Choose a reason for hiding this comment

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

//      on this node
//      and if so sets the flag GTF_DIV_BY_CNS_OPT and

it looks like an unintentional new line.

// and if so sets the flag GTF_DIV_BY_CNS_OPT and
// set GTF_DONT_CSE on the constant node
//
// Arguments:
// this - a GenTreeOp node
// comp - the compiler instance
//
void GenTreeOp::CheckDivideByConstOptimized(Compiler* comp)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want this to happen in minopts (opts.OptimizationEnabled() == false?

Copy link
Contributor Author

@briansull briansull Jul 9, 2020

Choose a reason for hiding this comment

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

I will add a check for minopts to UsesDivideByConstOptimization()

{
if (UsesDivideByConstOptimized(comp))
{
gtFlags |= GTF_DIV_BY_CNS_OPT;

// Now set DONT_CSE on the GT_CNS_INT divisor, note that
// with ValueNumbering we can have a non GT_CNS_INT divisior
GenTree* divisor = gtGetOp2()->gtEffectiveVal(/*commaOnly*/ true);
if (divisor->OperIs(GT_CNS_INT))
{
divisor->gtFlags |= GTF_DONT_CSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

you have answered earlier why we check GTF_DONT_CSE in optVNConstantPropOnJTrue and as I understood the idea was to replace constant values with CSE lclVars, is my understanding correct?
If so why do we forbid replacing these const with a CSE lclVar here?

Copy link
Contributor Author

@briansull briansull Jul 9, 2020

Choose a reason for hiding this comment

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

Because changing the CNS_INT to a CSE LclVar will cause lower to fail to use the multiplication by reciprocal optimization.
Thus we would generate a slow divide instruction instead of the faster multiply or shift sequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think I understand now.

From these two comments:

Doing a constant prop here would replace the CSE LclVar with the original constant.
Essentially undoing the CSE of the constant.

Because changing the CNS_INT to a CSE LclVar will cause lower to fail to use the multiplication by reciprocal optimization.

In general we want to set DONT_CSE on constants under DIV/MOD so they are not replaced with a CSE LCL_VAR. Other constants (that are not under 'DIV/MOD) are not marked as DONT_CSEso they could be replaced and, once they are replaced, we mark it withDONT_CSE` because it is their final state. Is it correct?

}
}
}

//
//------------------------------------------------------------------------
// gtBlockOpInit: Initializes a BlkOp GenTree
Expand Down Expand Up @@ -9899,6 +10062,18 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, __in __in_z _
}
goto DASH;

case GT_DIV:
case GT_MOD:
case GT_UDIV:
case GT_UMOD:
if (tree->gtFlags & GTF_DIV_BY_CNS_OPT)
{
printf("M"); // We will use a Multiply by reciprical
--msgLength;
break;
}
goto DASH;

case GT_LCL_FLD:
case GT_LCL_VAR:
case GT_LCL_VAR_ADDR:
Expand Down Expand Up @@ -10566,16 +10741,30 @@ void Compiler::gtDispConst(GenTree* tree)
else if ((tree->AsIntCon()->gtIconVal > -1000) && (tree->AsIntCon()->gtIconVal < 1000))
{
printf(" %ld", dspIconVal);
#ifdef TARGET_64BIT
}
#ifdef TARGET_64BIT
else if ((tree->AsIntCon()->gtIconVal & 0xFFFFFFFF00000000LL) != 0)
{
printf(" 0x%llx", dspIconVal);
#endif
if (dspIconVal >= 0)
{
printf(" 0x%llx", dspIconVal);
}
else
{
printf(" -0x%llx", -dspIconVal);
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting from VS2013 we have support for "%zd" (and other runtime parts are already using it), so I would suggest to just replace this block with:
printf(" %zd", dspIconVal);
that will handle both 32/64 and negative/positive.

Note: gcc and clang have always supported that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like see these numbers in hex, especially with my shared CSE constant changes where we often have to add or subtract a small offset.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not seen hex numbers being printed with a negative sign, but maybe it is fine. Thanks for the explanation.

}
}
#endif
else
{
printf(" 0x%X", dspIconVal);
if (dspIconVal >= 0)
{
printf(" 0x%X", dspIconVal);
}
else
{
printf(" -0x%X", -dspIconVal);
}
}

if (tree->IsIconHandle())
Expand Down
21 changes: 21 additions & 0 deletions src/coreclr/src/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,12 @@ struct GenTree
assert(_gtRegNum == reg);
}

void ClearRegNum()
{
_gtRegNum = REG_NA;
INDEBUG(gtRegTag = GT_REGTAG_NONE;)
}

// Copy the _gtRegNum/gtRegTag fields
void CopyReg(GenTree* from);
bool gtHasReg() const;
Expand Down Expand Up @@ -922,6 +928,8 @@ struct GenTree
#define GTF_OVERFLOW 0x10000000 // Supported for: GT_ADD, GT_SUB, GT_MUL and GT_CAST.
// Requires an overflow check. Use gtOverflow(Ex)() to check this flag.

#define GTF_DIV_BY_CNS_OPT 0x80000000 // GT_DIV -- Uses the division by constant optimization to compute this division

#define GTF_ARR_BOUND_INBND 0x80000000 // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds

#define GTF_ARRLEN_ARR_IDX 0x80000000 // GT_ARR_LENGTH -- Length which feeds into an array index expression
Expand Down Expand Up @@ -2853,6 +2861,19 @@ struct GenTreeOp : public GenTreeUnOp
assert(oper == GT_NOP || oper == GT_RETURN || oper == GT_RETFILT || OperIsBlk(oper));
}

// returns true if we will use the division by constant optimization for this node.
bool UsesDivideByConstOptimized(Compiler* comp);

// checks if we will use the division by constant optimization this node
// then sets the flag GTF_DIV_BY_CNS_OPT and GTF_DONT_CSE on the constant
void CheckDivideByConstOptimized(Compiler* comp);

// True if this node is marked as using the division by constant optimization
bool MarkedDivideByConstOptimized() const
{
return (gtFlags & GTF_DIV_BY_CNS_OPT) != 0;
}

#if DEBUGGABLE_GENTREE
GenTreeOp() : GenTreeUnOp(), gtOp2(nullptr)
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5134,6 +5134,7 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod)
unreached();
#endif
}
assert(divMod->MarkedDivideByConstOptimized());

// Depending on the "add" flag returned by GetUnsignedMagicNumberForDivide we need to generate:
// add == false (when divisor == 3 for example):
Expand Down Expand Up @@ -5207,7 +5208,6 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod)
BlockRange().InsertBefore(divMod, div, divisor, mul, dividend);
}
ContainCheckRange(firstNode, divMod);

return true;
}
#endif
Expand Down
Loading