Skip to content

JIT: Use jitstd::utility::scoped_code to unmark trees during lowering #106046

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 3 commits into from
Aug 7, 2024
Merged

JIT: Use jitstd::utility::scoped_code to unmark trees during lowering #106046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
878fc7d
Save/Restore flag state on Debug path
amanasifkhalid Aug 6, 2024
8b48e9f
Revert
amanasifkhalid Aug 6, 2024
d8ad774
Use scoped_code to unmark trees
amanasifkhalid Aug 6, 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
15 changes: 9 additions & 6 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9305,6 +9305,9 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
GenTree* cur = prevIndir;
for (int i = 0; i < LDP_STP_REORDERING_MAX_DISTANCE; i++)
{
// No nodes should be marked yet
assert((cur->gtLIRFlags & LIR::Flags::Mark) == 0);

cur = cur->gtNext;
if (cur == indir)
break;
Expand Down Expand Up @@ -9355,6 +9358,12 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi

#endif

// Unmark tree when we exit the current scope
auto code = [this, indir] {
UnmarkTree(indir);
};
jitstd::utility::scoped_code<decltype(code)> finally(code);
Comment on lines +9362 to +9365
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Aug 6, 2024

Choose a reason for hiding this comment

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

Suggested change
auto code = [this, indir] {
UnmarkTree(indir);
};
jitstd::utility::scoped_code<decltype(code)> finally(code);
jitstd::utility::scoped_code finally([this, indir] {
UnmarkTree(indir);
});


MarkTree(indir);

INDEBUG(dumpWithMarks());
Expand All @@ -9363,7 +9372,6 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
if ((prevIndir->gtLIRFlags & LIR::Flags::Mark) != 0)
{
JITDUMP("Previous indir is part of the data flow of current indir\n");
UnmarkTree(indir);
return false;
}

Expand All @@ -9379,7 +9387,6 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
if (m_scratchSideEffects.InterferesWith(comp, cur, true))
{
JITDUMP("Giving up due to interference with [%06u]\n", Compiler::dspTreeID(cur));
UnmarkTree(indir);
return false;
}

Expand All @@ -9401,7 +9408,6 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
if (!indir->OperIsLoad())
{
JITDUMP("Have conservative interference with last store. Giving up.\n");
UnmarkTree(indir);
return false;
}

Expand Down Expand Up @@ -9495,7 +9501,6 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
if (interferes(cur))
{
JITDUMP("Indir [%06u] interferes with [%06u]\n", Compiler::dspTreeID(indir), Compiler::dspTreeID(cur));
UnmarkTree(indir);
return false;
}
}
Expand All @@ -9521,7 +9526,6 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
{
JITDUMP("Cannot move prev indir [%06u] up past [%06u] to get it past the data computation\n",
Compiler::dspTreeID(prevIndir), Compiler::dspTreeID(cur));
UnmarkTree(indir);
return false;
}
}
Expand Down Expand Up @@ -9586,7 +9590,6 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi
JITDUMP("Result:\n\n");
INDEBUG(dumpWithMarks());
JITDUMP("\n");
UnmarkTree(indir);
return true;
}

Expand Down
12 changes: 9 additions & 3 deletions src/coreclr/jit/lowerarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,9 @@ bool Lowering::TryMakeIndirAndStoreAdjacent(GenTreeIndir* prevIndir, GenTreeLclV
GenTree* cur = prevIndir;
for (int i = 0; i < POST_INDEXED_ADDRESSING_MAX_DISTANCE; i++)
{
// No nodes should be marked yet
assert((cur->gtLIRFlags & LIR::Flags::Mark) == 0);

cur = cur->gtNext;
if (cur == store)
break;
Expand Down Expand Up @@ -1197,6 +1200,12 @@ bool Lowering::TryMakeIndirAndStoreAdjacent(GenTreeIndir* prevIndir, GenTreeLclV

#endif

// Unmark tree when we exit the current scope
auto code = [this, store] {
UnmarkTree(store);
};
jitstd::utility::scoped_code<decltype(code)> finally(code);
Comment on lines +1204 to +1207
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Aug 6, 2024

Choose a reason for hiding this comment

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

Suggested change
auto code = [this, store] {
UnmarkTree(store);
};
jitstd::utility::scoped_code<decltype(code)> finally(code);
jitstd::utility::scoped_code finally([this, store] {
UnmarkTree(store);
});


MarkTree(store);

INDEBUG(dumpWithMarks());
Expand All @@ -1214,7 +1223,6 @@ bool Lowering::TryMakeIndirAndStoreAdjacent(GenTreeIndir* prevIndir, GenTreeLclV
if (m_scratchSideEffects.InterferesWith(comp, cur, true))
{
JITDUMP("Giving up due to interference with [%06u]\n", Compiler::dspTreeID(cur));
UnmarkTree(store);
return false;
}
}
Expand All @@ -1229,7 +1237,6 @@ bool Lowering::TryMakeIndirAndStoreAdjacent(GenTreeIndir* prevIndir, GenTreeLclV
if (m_scratchSideEffects.InterferesWith(comp, store, true))
{
JITDUMP("Have interference. Giving up.\n");
UnmarkTree(store);
return false;
}

Expand Down Expand Up @@ -1260,7 +1267,6 @@ bool Lowering::TryMakeIndirAndStoreAdjacent(GenTreeIndir* prevIndir, GenTreeLclV
JITDUMP("Result:\n\n");
INDEBUG(dumpWithMarks());
JITDUMP("\n");
UnmarkTree(store);
return true;
}

Expand Down