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.

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

JIT: Regularize readbacks for parameters/OSR-locals in physical promotion #87165

Merged
merged 5 commits into from
Jun 17, 2023

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Jun 6, 2023

Handle readbacks for parameters/OSR-locals like any other readback is
handled. Previously they were handled by creating the scratch BB and
then inserting IR after the main replacement had already been done; now,
we instead create the scratch BB eagerly and mark these as requiring a
read back at the beginning of the scratch BB, and leave normal
replacement logic up to handle it.

The main benefit is that this unification makes it easier to ensure that
future smarter handling of readbacks/writebacks (e.g. "resolution")
automatically kicks in for the common case of parameters.

Introduce another invariant, which is that we only ever mark a field as
requiring readback if it is live. Previously we would always mark them
as requiring readbacks, but would then check liveness before inserting
the actual IR to do the readback. But we don't always have the liveness
information at the point where we insert IR for readbacks after #86706.

Also expand some debug logging, and address some feedback from #86706.

…tion

Handle readbacks for parameters/OSR-locals like any other readback is
handled. Previously they were handled by creating the scratch BB and
then inserting IR after the main replacement had already been done; now,
we instead create the scratch BB eagerly and mark these as requiring a
read back at the beginning of the scratch BB, and leave normal
replacement logic up to handle it.

The main benefit is that this unification makes it easier to ensure that
future smarter handling of readbacks/writebacks (e.g. "resolution")
automatically kicks in for the common case of parameters.

Introduce another invariant, which is that we only ever mark a field as
requiring readback if it is live. Previously we would always mark them
as requiring read backs, but would then check liveness before inserting
the actual IR to do the read back. But we don't always have the liveness
information at the point where we insert IR for readbacks after dotnet#86706.

Also expand some debug logging, and address some feedback from dotnet#86706.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 6, 2023
@ghost ghost assigned jakobbotsch Jun 6, 2023
@ghost
Copy link

ghost commented Jun 6, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Handle readbacks for parameters/OSR-locals like any other readback is
handled. Previously they were handled by creating the scratch BB and
then inserting IR after the main replacement had already been done; now,
we instead create the scratch BB eagerly and mark these as requiring a
read back at the beginning of the scratch BB, and leave normal
replacement logic up to handle it.

The main benefit is that this unification makes it easier to ensure that
future smarter handling of readbacks/writebacks (e.g. "resolution")
automatically kicks in for the common case of parameters.

Introduce another invariant, which is that we only ever mark a field as
requiring readback if it is live. Previously we would always mark them
as requiring read backs, but would then check liveness before inserting
the actual IR to do the read back. But we don't always have the liveness
information at the point where we insert IR for readbacks after #86706.

Also expand some debug logging, and address some feedback from #86706.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

Small number of diffs expected. They appear when there is an existing scratch block with some IR (typically a cctor invocation) and we now insert readbacks after that IR instead of before. That leads to some different register allocation.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch
Copy link
Member Author

jitstress restore failed with #84995... will restart it.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

jitstress failures are #87258 and #84911.

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS. Few diffs as mentioned above.

Diffs with physical promotion, diffs without old promotion. I was mainly worried about TP of having another basic block to compute liveness for in these cases, but that doesn't seem to be costly.

@jakobbotsch jakobbotsch marked this pull request as ready for review June 8, 2023 09:27
Comment on lines 690 to +703
// Retbuf -- these are definitions but we do not know of how much.
// We never mark them as dead and we never treat them as killing anything.
assert(isDef);
// We never treat them as killing anything, but we do store liveness information for them.
BitVecTraits aggTraits(1 + (unsigned)agg->Replacements.size(), m_compiler);
BitVec aggDeaths(BitVecOps::MakeEmpty(&aggTraits));
// Copy preexisting liveness information.
for (size_t i = 0; i <= agg->Replacements.size(); i++)
{
unsigned varIndex = baseIndex + (unsigned)i;
if (!BitVecOps::IsMember(m_bvTraits, life, varIndex))
{
BitVecOps::AddElemD(&aggTraits, aggDeaths, (unsigned)i);
}
}
m_aggDeaths.Set(lcl, aggDeaths);
Copy link
Member Author

Choose a reason for hiding this comment

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

We need this liveness here now to know what fields we need to mark for readback when a physically promoted struct is passed as the retbuf.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch jakobbotsch merged commit 2c62994 into dotnet:main Jun 17, 2023
@jakobbotsch jakobbotsch deleted the lazy-readbacks branch June 17, 2023 18:38
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants