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

Changes from 1 commit
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
35 changes: 35 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,41 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)

if (conValTree != nullptr)
{
if (tree->OperIs(GT_LCL_VAR))
{
bool betterToPropagate = false;
if (conValTree->IsVectorZero() || conValTree->IsFloatPositiveZero())
{
// These are cheap to re-materialize (unlike e.g. "1.0" or "{1,1,1,1}" which end up as
// memory loads from the data section)
betterToPropagate = true;
}
#ifdef TARGET_ARM64
else if (conValTree->IsCnsIntOrI() && unsigned_abs(conValTree->AsIntCon()->IconValue()) < 0x0fff)
{
// This integer constant is likely immable
betterToPropagate = true;
}
#endif

if (!betterToPropagate)
{
// Try to find the block this value was defined in
// and if the current use is inside a loop while the def is not - do not propagate
unsigned lclNum = tree->AsLclVarCommon()->GetLclNum();
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 && !(defBlock->bbFlags & BBF_BACKWARD_JUMP) &&
(block->bbFlags & BBF_BACKWARD_JUMP))
{
return nullptr;
}
}
}
}

// Were able to optimize.
conValTree->gtVNPair = vnPair;
GenTree* sideEffList = optExtractSideEffListFromConst(tree);
Expand Down