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

Delete GT_DYN_BLK #63026

Merged
merged 7 commits into from
Jan 21, 2022
Merged

Delete GT_DYN_BLK #63026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
de84dc4
Import GT_STORE_DYN_BLK directly
SingleAccretion Nov 24, 2021
f67b4b3
Delete GT_DYN_BLK
SingleAccretion Nov 25, 2021
af3f667
DynBlk -> StoreDynBlk
SingleAccretion Dec 19, 2021
fecc339
Add some tests
SingleAccretion Dec 19, 2021
86d4b95
Mark tests Pri-1
SingleAccretion Dec 21, 2021
d3263b6
Rebase and fix build
SingleAccretion Jan 21, 2022
36f4353
Bring back the odd early return
SingleAccretion Jan 21, 2022
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
3 changes: 1 addition & 2 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2576,7 +2576,6 @@ void Compiler::optAssertionGen(GenTree* tree)

case GT_OBJ:
case GT_BLK:
case GT_DYN_BLK:
case GT_IND:
// R-value indirections create non-null assertions, but not all indirections are R-values.
// Those under ADDR nodes or on the LHS of ASGs are "locations", and will not end up
Expand Down Expand Up @@ -4626,9 +4625,9 @@ GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree,

case GT_OBJ:
case GT_BLK:
case GT_DYN_BLK:
case GT_IND:
case GT_NULLCHECK:
case GT_STORE_DYN_BLK:
return optAssertionProp_Ind(assertions, tree, stmt);

case GT_BOUNDS_CHECK:
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1912,7 +1912,7 @@ void CodeGen::genSetBlockSize(GenTreeBlk* blkNode, regNumber sizeReg)
}
else
{
GenTree* sizeNode = blkNode->AsDynBlk()->gtDynamicSize;
GenTree* sizeNode = blkNode->AsStoreDynBlk()->gtDynamicSize;
inst_Mov(sizeNode->TypeGet(), sizeReg, sizeNode->GetRegNum(), /* canSkip */ true);
}
}
Expand Down Expand Up @@ -2022,7 +2022,7 @@ void CodeGen::genConsumeBlockOp(GenTreeBlk* blkNode, regNumber dstReg, regNumber
// in the case where the size is a constant (i.e. it is not GT_STORE_DYN_BLK).
if (blkNode->OperGet() == GT_STORE_DYN_BLK)
{
genConsumeReg(blkNode->AsDynBlk()->gtDynamicSize);
genConsumeReg(blkNode->AsStoreDynBlk()->gtDynamicSize);
}

// Next, perform any necessary moves.
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9389,7 +9389,6 @@ void cTreeFlags(Compiler* comp, GenTree* tree)
FALLTHROUGH;

case GT_BLK:
case GT_DYN_BLK:
case GT_STORE_BLK:
case GT_STORE_DYN_BLK:

Expand Down Expand Up @@ -9742,7 +9741,7 @@ bool Compiler::lvaIsOSRLocal(unsigned varNum)
//
void Compiler::gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block)
{
assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK, GT_DYN_BLK));
assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK));
tree->ChangeOper(GT_NULLCHECK);
tree->ChangeType(TYP_INT);
block->bbFlags |= BBF_HAS_NULLCHECK;
Expand Down
31 changes: 2 additions & 29 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6404,6 +6404,7 @@ class Compiler
GenTree* fgMorphGetStructAddr(GenTree** pTree, CORINFO_CLASS_HANDLE clsHnd, bool isRValue = false);
GenTree* fgMorphBlockOperand(GenTree* tree, var_types asgType, unsigned blockWidth, bool isBlkReqd);
GenTree* fgMorphCopyBlock(GenTree* tree);
GenTree* fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree);
GenTree* fgMorphForRegisterFP(GenTree* tree);
GenTree* fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac = nullptr);
GenTree* fgOptimizeCast(GenTreeCast* cast);
Expand Down Expand Up @@ -11500,42 +11501,14 @@ class GenTreeVisitor
break;
}

case GT_DYN_BLK:
{
GenTreeDynBlk* const dynBlock = node->AsDynBlk();

GenTree** op1Use = &dynBlock->gtOp1;
GenTree** op2Use = &dynBlock->gtDynamicSize;

result = WalkTree(op1Use, dynBlock);
if (result == fgWalkResult::WALK_ABORT)
{
return result;
}
result = WalkTree(op2Use, dynBlock);
if (result == fgWalkResult::WALK_ABORT)
{
return result;
}
break;
}

case GT_STORE_DYN_BLK:
{
GenTreeDynBlk* const dynBlock = node->AsDynBlk();
GenTreeStoreDynBlk* const dynBlock = node->AsStoreDynBlk();

GenTree** op1Use = &dynBlock->gtOp1;
GenTree** op2Use = &dynBlock->gtOp2;
GenTree** op3Use = &dynBlock->gtDynamicSize;

if (TVisitor::UseExecutionOrder)
{
if (dynBlock->IsReverseOp())
{
std::swap(op1Use, op2Use);
}
}

Comment on lines -11531 to -11538
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I haven't seen it yet, but why get rid of the reverse ops option? Isn't that used to improve codegen if the order doesn't matter, so we can execute the heavier op first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was dead code - nothing sets GTF_REVERSE_OPS on a STORE_DYN_BLK node. GTF_REVERSE_OPS is only supported for (most) binary nodes and 2-operand GenTreeMultiOps.

However, what did happen is that we reversed ASGs that would later turn into STORE_DYN_BLKs and now we don't. The handling could be added, but it would have to be "non-standard" because STORE_DYN_BLK is a tertiary operator, and I believe, based on the diffs for this change, that it would not be worth it at this point.

Side note: we could get all the 6 permutations of order "for free" if we changed STORE_DYN_BLK from a tertiary operator with DynamicSize as its member into two binary operators: STORE_DYN_BLK(DstAddr, Data) where Data would be DYN_BLK(Addr, Size) | DYN_BLK_INIT_VAL(InitVal, Size), and also solve the problem with the "dummy" IND struct node. But as I note in the description it would be a larger change with quite a bit of churn to it.

result = WalkTree(op1Use, dynBlock);
if (result == fgWalkResult::WALK_ABORT)
{
Expand Down
13 changes: 1 addition & 12 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4347,20 +4347,9 @@ void GenTree::VisitOperands(TVisitor visitor)
return;
}

case GT_DYN_BLK:
{
GenTreeDynBlk* const dynBlock = this->AsDynBlk();
if (visitor(dynBlock->gtOp1) == VisitResult::Abort)
{
return;
}
visitor(dynBlock->gtDynamicSize);
return;
}

case GT_STORE_DYN_BLK:
{
GenTreeDynBlk* const dynBlock = this->AsDynBlk();
GenTreeStoreDynBlk* const dynBlock = this->AsStoreDynBlk();
if (visitor(dynBlock->gtOp1) == VisitResult::Abort)
{
return;
Expand Down
32 changes: 3 additions & 29 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3910,33 +3910,6 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR)
return;
}

// Special handling for dynamic block ops.
if (tree->OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK))
{
GenTreeDynBlk* dynBlk = tree->AsDynBlk();
GenTree* sizeNode = dynBlk->gtDynamicSize;
GenTree* dstAddr = dynBlk->Addr();
GenTree* src = dynBlk->Data();
bool isReverse = dynBlk->IsReverseOp();

// We either have a DYN_BLK or a STORE_DYN_BLK. If the latter, we have a
// src (the Data to be stored), and isReverse tells us whether to evaluate
// that before dstAddr.
if (isReverse && (src != nullptr))
{
fgSetTreeSeqHelper(src, isLIR);
}
fgSetTreeSeqHelper(dstAddr, isLIR);
if (!isReverse && (src != nullptr))
{
fgSetTreeSeqHelper(src, isLIR);
}
fgSetTreeSeqHelper(sizeNode, isLIR);

fgSetTreeSeqFinish(dynBlk, isLIR);
return;
}

/* Is it a 'simple' unary/binary operator? */

if (kind & GTK_SMPOP)
Expand Down Expand Up @@ -4108,8 +4081,9 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR)
break;

case GT_STORE_DYN_BLK:
case GT_DYN_BLK:
noway_assert(!"DYN_BLK nodes should be sequenced as a special case");
fgSetTreeSeqHelper(tree->AsStoreDynBlk()->Addr(), isLIR);
fgSetTreeSeqHelper(tree->AsStoreDynBlk()->Data(), isLIR);
fgSetTreeSeqHelper(tree->AsStoreDynBlk()->gtDynamicSize, isLIR);
break;

default:
Expand Down
Loading