Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Outdated] Implementation of CSE for GT_CNS_INT benefits ARM64. #36831

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a7a3706
Mark Div/Mod nodes that will use reciprocal multiply with GTF_DIV_USE…
briansull Apr 24, 2020
0c89250
Update the LclVar ref counts for new CSE LclVars
briansull May 14, 2020
e1716fa
Change csdHashKey to INT64, change the CSE hash key to size_t
briansull May 14, 2020
fc4cddb
Update usesMagicNumberDivision() to handle some additional cases
briansull May 14, 2020
333ceef
Update the gtCost values for constant nodes: GT_CNS_INT, GT_CNS_LNG, …
briansull May 14, 2020
33fb8b0
Fix usesMagicNumberDivision for unsigned divide of MIN_INT
briansull May 16, 2020
5d017f8
If a tree node has had SetRegNum() called to assign it to a physical …
briansull May 20, 2020
9d87148
As part of the CSE of constants work, once we make a CSE we don't wan…
briansull May 20, 2020
f427256
When we duplicate a conditional test to change a loop into a bottom t…
briansull May 20, 2020
140d6dd
In invariant hoisting the hoist Expr does not have to computed into a…
briansull May 21, 2020
29d02ea
Change the weights of GT_CNS_INT for ARM64
briansull May 21, 2020
e778781
Don't auto retry when using the AltJit as that results in two copies …
briansull May 21, 2020
84a208c
Implementation of CSE for GT_CNS_INT using a single CSE for constant …
briansull May 21, 2020
d5ccbdb
Fix for JitDisableConstCSE() == 3
briansull Jun 12, 2020
5b8b9bd
In CSE added check for mismatched types on GT_CNS_INT nodes
briansull Jun 17, 2020
5a8fac2
Fix assert divMod->markedMagicNumberDivision()
briansull Jun 23, 2020
5effd8e
Propagate the gtFlags for a gtCallAddr node
briansull Jun 23, 2020
d6ed1b1
Propagating a constant may create an opportunity to use a magic numbe…
briansull Jun 25, 2020
640e945
Only perform conversion of floating point GT_DIV to an exact reciproc…
briansull Jun 25, 2020
2240392
Mark the non-standard virtual stub indirection address arg with GTF_D…
briansull Jun 25, 2020
0d84d8c
Changes for config variable JitDisableConstCSE - remove values 4 and 5
briansull Jun 30, 2020
78ae001
Allow CSE of the R2R indirect param constant address
briansull Jun 30, 2020
34079eb
Fix Assertion failed 'isValidGeneralDatasize(size)'
briansull Jul 1, 2020
baf350e
Change gtCostEx for 8-byte constants on AMD64 to be 2 so that they ar…
briansull Jul 3, 2020
0f66687
Workaround for ARM32 assert when CSE of Consts is enabled
briansull Jul 3, 2020
421fd5a
Add a simple sort to compute the best shared const CSE's to use for t…
briansull Jul 3, 2020
4b429fd
Fix the gtCost GT_CNS_LNG for ARM32
briansull Jun 25, 2020
8340e81
Update arm32 GT_CNS costs.
briansull Jun 27, 2020
83b5dc2
Fix in AssertionProp for JitStress issue with beq_r8 test
briansull Jul 7, 2020
435bdd5
Added additional Priority 0 test coverage for Floating Point optimiza…
briansull Jul 8, 2020
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a correctness fix? Were we getting wrong VN pair for != in the past?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is incorrectly assigning the wrong constant value number. I found similar code
It was occasionally happening in the framework dlls and the the beq_r8 test cases.
Our code has two chances to fold the expression, the first one will fold compares with two constant arguments,
which is the common case. If that doesn't fold then it tries folding using the value numbers and that path was wrong and caused the beq_r8 tests to fail with my CSE changes.

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())
Copy link
Contributor

Choose a reason for hiding this comment

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

was it unprofitable or incorrect to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
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
18 changes: 17 additions & 1 deletion src/coreclr/src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -6320,7 +6320,10 @@ class Compiler
{
CSEdsc* csdNextInBucket; // used by the hash table

unsigned csdHashKey; // the orginal hashkey
size_t csdHashKey; // the orginal hashkey
ssize_t csdConstDefValue; // When we CSE similar constants this is the value that we use as the def
ValueNum csdConstDefVN; // When we CSE similar constants this is the ValueNumber that we use for the LclVar
// assignment

unsigned csdIndex; // 1..optCSECandidateCount
bool csdLiveAcrossCall;
Expand Down Expand Up @@ -6353,6 +6356,19 @@ class Compiler
// number, this will reflect it; otherwise, NoVN.
};

#if defined(TARGET_XARCH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this block live in target.h?

#define CSE_CONST_SHARED_LOW_BITS 16
#else
// ARM64 or ARM32
#define CSE_CONST_SHARED_LOW_BITS 12
#endif

#ifdef TARGET_64BIT
#define TARGET_SIGN_BIT (1ULL << 63)
#else
#define TARGET_SIGN_BIT (1ULL << 31)
#endif

static const size_t s_optCSEhashSize;
CSEdsc** optCSEhash;
CSEdsc** optCSEtab;
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 magic number division
//
if ((tree->gtNext != nullptr) && tree->gtNext->OperIsBinary())
{
// We need to mark the parent divide/mod operation when this occurs
tree->gtNext->AsOp()->checkMagicNumberDivision(this);
}

#ifdef DEBUG
if (verbose)
{
Expand Down
Loading