Skip to content

JIT: enhance dead stores for locals where all uses are useasg #115031

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

Closed
wants to merge 3 commits into from
Closed

JIT: enhance dead stores for locals where all uses are useasg #115031

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92cf121
JIT: enhance dead stores for locals where all uses are useasg
AndyAyersMS Apr 25, 2025
8a2153a
account for tailcall/jmp uses
AndyAyersMS Apr 25, 2025
4366b78
also mark true uses of promoted fields
AndyAyersMS Apr 26, 2025
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
5 changes: 3 additions & 2 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6709,8 +6709,9 @@ class Compiler

//----------------------- Liveness analysis -------------------------------

VARSET_TP fgCurUseSet; // vars used by block (before a def)
VARSET_TP fgCurDefSet; // vars assigned by block (before a use)
VARSET_TP fgCurUseSet; // vars used by block (before a def)
VARSET_TP fgCurDefSet; // vars assigned by block (before a use)
VARSET_TP fgTrueUseSet; // vars with non-useasg uses

MemoryKindSet fgCurMemoryUse; // True iff the current basic block uses memory.
MemoryKindSet fgCurMemoryDef; // True iff the current basic block modifies memory.
Expand Down
24 changes: 22 additions & 2 deletions src/coreclr/jit/liveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ void Compiler::fgMarkUseDef(GenTreeLclVarCommon* tree)
// This is a def, add it to the set of defs.
VarSetOps::AddElemD(this, fgCurDefSet, varDsc->lvVarIndex);
}

if (isUse && !isDef)
{
// Variable is actually used
VarSetOps::AddElemD(this, fgTrueUseSet, varDsc->lvVarIndex);
}
}
else
{
Expand Down Expand Up @@ -371,6 +377,7 @@ void Compiler::fgPerBlockLocalVarLiveness()
// Avoid allocations in the long case.
VarSetOps::AssignNoCopy(this, fgCurUseSet, VarSetOps::MakeEmpty(this));
VarSetOps::AssignNoCopy(this, fgCurDefSet, VarSetOps::MakeEmpty(this));
VarSetOps::AssignNoCopy(this, fgTrueUseSet, VarSetOps::MakeEmpty(this));

// GC Heap and ByrefExposed can share states unless we see a def of byref-exposed
// memory that is not a GC Heap def.
Expand Down Expand Up @@ -910,7 +917,8 @@ bool Compiler::fgComputeLifeTrackedLocalDef(VARSET_TP& life,
assert(varDsc.lvTracked);

const unsigned varIndex = varDsc.lvVarIndex;
if (VarSetOps::IsMember(this, life, varIndex))
bool isLive = VarSetOps::IsMember(this, life, varIndex);
if (isLive)
{
// The variable is live
if ((node->gtFlags & GTF_VAR_USEASG) == 0)
Expand All @@ -932,8 +940,20 @@ bool Compiler::fgComputeLifeTrackedLocalDef(VARSET_TP& life,
}
#endif // DEBUG
}
else
{
// If the only uses are useasg defs, then the variable is not live unless artificially kept alive.
isLive =
VarSetOps::IsMember(this, fgTrueUseSet, varIndex) || VarSetOps::IsMember(this, keepAliveVars, varIndex);
Copy link
Member

Choose a reason for hiding this comment

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

If I understand right the fgTrueUseSet computed is a global set of variables and is not computed via data flow. Do you know how conservative that is, compared to making the data flow computation more precise?

I would be interesting in which liveness phase (early liveness, SSA's liveness or lowering's liveness) actually end up removing these stores. If it is not removed by SSA's liveness then I think we can do better by making the liveness computations more precise (similar to physical promotion's liveness).

Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity I opened a PR for my old change: #115081

Copy link
Member Author

Choose a reason for hiding this comment

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

Spot checking I see some during SSA and some during Lower.

Let me add some metrics and see if I can get a more comprehensive picture.

Copy link
Member

@jakobbotsch jakobbotsch Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

#115081 does seem to get most of the same diffs and a few more. Given that it does not require any new accounting I would be inclined to suggest that approach instead. I would also imagine it does not have any TP diffs once things are templated correctly, but that does require a bit of clean up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine by me. Are you going to finish off your version?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let me try to do that.


if (!isLive)
{
VarSetOps::RemoveElemD(this, life, varIndex);
}
}
}
else

if (!isLive)
{
// Dead store
node->gtFlags |= GTF_VAR_DEATH;
Expand Down
Loading