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.

Sign up for GitHub

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

Jump to bottom

[JIT] Allow side-effects for the first test in switch recognition #106988

Merged
merged 3 commits into from
Aug 29, 2024
Merged

[JIT] Allow side-effects for the first test in switch recognition #106988

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c710e65
allow side-effects for the first test in switch
EgorBo Aug 26, 2024
90f3762
Update switchrecognition.cpp
EgorBo Aug 27, 2024
5fa8752
Update switchrecognition.cpp
EgorBo Aug 27, 2024
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
34 changes: 22 additions & 12 deletions src/coreclr/jit/switchrecognition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,19 @@ PhaseStatus Compiler::optSwitchRecognition()
// constant test? e.g. JTRUE(EQ/NE(X, CNS)).
//
// Arguments:
// block - The block to check
// trueEdge - [out] The successor edge taken if X == CNS
// falseEdge - [out] The successor edge taken if X != CNS
// isReversed - [out] True if the condition is reversed (GT_NE)
// variableNode - [out] The variable node (X in the example above)
// cns - [out] The constant value (CNS in the example above)
// block - The block to check
// allowSideEffects - is variableNode allowed to have side-effects (COMMA)?
// trueEdge - [out] The successor edge taken if X == CNS
// falseEdge - [out] The successor edge taken if X != CNS
// isReversed - [out] True if the condition is reversed (GT_NE)
// variableNode - [out] The variable node (X in the example above)
// cns - [out] The constant value (CNS in the example above)
//
// Return Value:
// True if the block represents a constant test, false otherwise
//
bool IsConstantTestCondBlock(const BasicBlock* block,
bool allowSideEffects,
BasicBlock** trueTarget,
BasicBlock** falseTarget,
bool* isReversed,
Expand Down Expand Up @@ -89,7 +91,15 @@ bool IsConstantTestCondBlock(const BasicBlock* block,
if ((op1->IsCnsIntOrI() && !op1->IsIconHandle()) ^ (op2->IsCnsIntOrI() && !op2->IsIconHandle()))
{
// TODO: relax this to support any side-effect free expression
Copy link
Member

Choose a reason for hiding this comment

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

Is there any more side effect to support other than COMMA? If not, please remove the TODO comment becasue this PR will support COMMA side-effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

if (!op1->OperIs(GT_LCL_VAR) && !op2->OperIs(GT_LCL_VAR))

if (allowSideEffects)
{
if (!op1->gtEffectiveVal()->OperIs(GT_LCL_VAR) && !op2->gtEffectiveVal()->OperIs(GT_LCL_VAR))
{
return false;
}
}
else if (!op1->OperIs(GT_LCL_VAR) && !op2->OperIs(GT_LCL_VAR))
{
return false;
}
Expand Down Expand Up @@ -148,7 +158,7 @@ bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock)
// and then try to accumulate as many constant test blocks as possible. Once we hit
// a block that doesn't match the pattern, we start processing the accumulated blocks.
bool isReversed = false;
if (IsConstantTestCondBlock(firstBlock, &trueTarget, &falseTarget, &isReversed, &variableNode, &cns))
if (IsConstantTestCondBlock(firstBlock, true, &trueTarget, &falseTarget, &isReversed, &variableNode, &cns))
{
if (isReversed)
{
Expand Down Expand Up @@ -185,16 +195,16 @@ bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock)
}

// Inspect secondary blocks
if (IsConstantTestCondBlock(currBb, &currTrueTarget, &currFalseTarget, &isReversed, &currVariableNode,
&currCns))
if (IsConstantTestCondBlock(currBb, false, &currTrueTarget, &currFalseTarget, &isReversed,
&currVariableNode, &currCns))
{
if (currTrueTarget != trueTarget)
{
// This blocks jumps to a different target, stop searching and process what we already have.
return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
}

if (!GenTree::Compare(currVariableNode, variableNode))
if (!GenTree::Compare(currVariableNode, variableNode->gtEffectiveVal()))
{
// A different variable node is used, stop searching and process what we already have.
return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
Expand Down Expand Up @@ -324,7 +334,7 @@ bool Compiler::optSwitchConvert(
BasicBlock* blockIfTrue = nullptr;
BasicBlock* blockIfFalse = nullptr;
bool isReversed = false;
const bool isTest = IsConstantTestCondBlock(lastBlock, &blockIfTrue, &blockIfFalse, &isReversed);
const bool isTest = IsConstantTestCondBlock(lastBlock, false, &blockIfTrue, &blockIfFalse, &isReversed);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we clone nodeToTest below? Initially it seemed like we were cloning the side effects too, but it looks like we discard the old version, so no cloning would be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this clone was an artifact from the initial impl, I have removed it in the latest commit.

assert(isTest);

assert(firstBlock->TrueTargetIs(blockIfTrue));
Expand Down
Loading