Skip to content

JIT: Do not propagate some constants #70378

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

Merged
merged 11 commits into from
Jun 10, 2022
Merged

JIT: Do not propagate some constants #70378

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
593a3ec
Do not propagate some constants
EgorBo Jun 7, 2022
72ab579
Limit to vectors for now
EgorBo Jun 8, 2022
83fc086
Extend impl for other constants
EgorBo Jun 8, 2022
d81b195
Clean up
EgorBo Jun 8, 2022
1d076dd
Remove integers for now
EgorBo Jun 8, 2022
0274989
Merge branch 'main' of github.com:dotnet/runtime into fix-cns-prop
EgorBo Jun 9, 2022
69e4d1d
Address feedback
EgorBo Jun 9, 2022
3920a54
Move to a function
EgorBo Jun 9, 2022
7873570
Remove unused method
EgorBo Jun 9, 2022
bc40d08
fix build
EgorBo Jun 9, 2022
eaede4e
Update src/coreclr/jit/assertionprop.cpp
EgorBo Jun 10, 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
61 changes: 61 additions & 0 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3177,6 +3177,67 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)

if (conValTree != nullptr)
{
if (tree->OperIs(GT_LCL_VAR) && conValTree->OperIs(GT_CNS_VEC, GT_CNS_DBL))
{
// A simple heuristic: If the constant is defined outside of a loop (not far from its head)
// and is used inside it - don't propagate.

bool keepPropagating = false;

if (conValTree->OperIs(GT_CNS_VEC))
{
if (conValTree->IsVectorZero() || conValTree->IsVectorAllBitsSet())
{
// These are cheap to materialize
keepPropagating = true;
}
}
else if (conValTree->OperIs(GT_CNS_DBL))
{
if (conValTree->IsFloatPositiveZero())
{
// This is cheap to materialize
keepPropagating = true;
}
#if defined(TARGET_ARM64)
if (emitter::canEncodeFloatImm8(conValTree->AsDblCon()->gtDconVal))
{
// Such floats are likely immable on arm64
keepPropagating = true;
}
#endif
}

if (!keepPropagating)
{
// Try to find the block this constant was originally defined in
const unsigned lclNum = tree->AsLclVarCommon()->GetLclNum();
const unsigned ssaNum = GetSsaNumForLocalVarDef(tree);
if (ssaNum != SsaConfig::RESERVED_SSA_NUM)
{
BasicBlock* defBlock = lvaTable[lclNum].GetPerSsaData(ssaNum)->GetBlock();
Copy link
Contributor

@SingleAccretion SingleAccretion Jun 9, 2022

Choose a reason for hiding this comment

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

Style nit: lvaTable[lclNum]. -> lvaGetDesc(lclNum)->.

General comment on the algorithm: this walks only one step up in the def chain, so if we have something like this:

var a = const;
for (...) {
    var b = a;
    Use(b);
}

We will still propagate const to Use.

Copy link
Member Author

@EgorBo EgorBo Jun 9, 2022

Choose a reason for hiding this comment

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

@SingleAccretion I don't see why it would.
When we're at Use(b) and we query SsaData()->Block from b we get a's Block and it leads to "avoid propagating" path

Copy link
Member Author

@EgorBo EgorBo Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, actually for your case b is going to be replaced with a in an earlier phase VN based copy prop

Copy link
Contributor

@SingleAccretion SingleAccretion Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When we're at Use(b) and we query SsaData()->Block from b we get a's Block and it leads to "avoid propagating" path

If I annotate this with blocks:

BB01:
var a = const;

for (...) {
BB02:
    var b = a; // b:d:1
    Use(b);    // b:u:1
}

I am pretty sure that when we query the def block for b:u:1, we'd get BB02, since the def b:d:1 occurs inside it.

Ah, actually for your case b is going to be replaced with a in an earlier phase VN based copy prop

I agree that copy propagation helps us here; not sure by how much because of the liveness constraints it has.

In any case, I also agree that there is no reason, for now, to complicate the code with a full UD chain walk, because we are pretty much targeting people hoisting constants out of loops by hands.

if (defBlock != nullptr)
{
// TODO: if it lives across calls in that loops we should propagate it only after those
// calls on ABIs without callee-saved registers to avoid spills
if (!defBlock->IsInLoop() && block->IsInLoop())
{
// Last, let's limit it to cases where block is not some not-always-taken
// block inside that loop
const weight_t defBlockWeight = defBlock->getBBWeight(this);
const weight_t blockWeight = block->getBBWeight(this);

if ((defBlockWeight > 0) && ((blockWeight / defBlockWeight) >= BB_LOOP_WEIGHT_SCALE))
{
JITDUMP("Skip constant propagation inside loop " FMT_BB "\n", block->bbNum);
return nullptr;
}
}
}
}
}
}

// Were able to optimize.
conValTree->gtVNPair = vnPair;
GenTree* sideEffList = optExtractSideEffListFromConst(tree);
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,11 @@ struct BasicBlock : private LIR::Range
return PredEdgeList(bbPreds);
}

bool IsInLoop() const
{
return bbNatLoopNum != NOT_IN_LOOP;
}

// PredBlocks: convenience method for enabling range-based `for` iteration over predecessor blocks, e.g.:
// for (BasicBlock* const predBlock : block->PredBlocks()) ...
//
Expand Down