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: revise local assertion prop to use bit vectors #94322

Merged

Conversation

AndyAyersMS
Copy link
Member

Track the set of active local assertions via a bit vector, rather than assuming all entries in the table are live.

Doing so required a number of changes in assertion prop to ensure the vector is consulted before deciding an assertion is valid.

This will (eventually) allow us to propagate assertions cross-block. For now we reset the bit vector and assertion table back to empty at the start of each block so nothing propagates past the end of a block.

The table can fill and cause the JIT to miss assertions in very large blocks as morph will no longer remove assertions while processing a block. Previously this would happen if there were more than 64 live assertions in a block, and now it can happen if there are more than 64 assertions in block (so somewhat more frequently).

Contributes to #93246.

…n tracking

Track the set of active local assertions via a bit vector, rather than assuming
all entries in the table are live.

Doing so required a number of changes in assertion prop to ensure the vector
is consulted before deciding an assertion is valid.

This will (eventually) allow us to propagate assertions cross-block. For
now we reset the bit vector and assertion table back to empty at the start
of each block so nothing propagates past the end of a block.

The table can fill and cause the JIT to miss assertions in very large blocks
as morph will no longer remove assertions while processing a block. Previously
this would happen if there were more than 64 live assertions in a block, and
now it can happen if there are more than 64 assertions in block (so somewhat
more frequently).

Contributes to dotnet#93246.
@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 Nov 2, 2023
@ghost ghost assigned AndyAyersMS Nov 2, 2023
@ghost
Copy link

ghost commented Nov 2, 2023

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

Issue Details

Track the set of active local assertions via a bit vector, rather than assuming all entries in the table are live.

Doing so required a number of changes in assertion prop to ensure the vector is consulted before deciding an assertion is valid.

This will (eventually) allow us to propagate assertions cross-block. For now we reset the bit vector and assertion table back to empty at the start of each block so nothing propagates past the end of a block.

The table can fill and cause the JIT to miss assertions in very large blocks as morph will no longer remove assertions while processing a block. Previously this would happen if there were more than 64 live assertions in a block, and now it can happen if there are more than 64 assertions in block (so somewhat more frequently).

Contributes to #93246.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Nov 2, 2023

@jakobbotsch PTAL
cc @dotnet/jit-contrib

Should cause a few hundred diffs, mainly in test code. Two RWC methods with big diffs; the next stage of this work may or may not help here as I will have to make the table larger....

Local TP impact looks good; let's see what the lab says.

Diffs

Not sure what to make of the linux TP numbers...

@BruceForstall
Copy link
Member

Not sure what to make of the linux TP numbers...

clang is not as good as MSVC at bit vector manipulation?

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM. I took the liberty of collecting a details TP diff on Linux, since I had a readily available setup to do that:

Base: 20039267461, Diff: 20129180266, +0.4487%

Compiler::fgMorphTreeDone(GenTree*, bool, bool)                                                                                     : 92335191 : NA      : 70.82% : +0.4608%
Compiler::fgMorphBlock(BasicBlock*)                                                                                                 : 7141209  : NA      : 5.48%  : +0.0356%
Compiler::fgMorphSmpOp(GenTree*, Compiler::MorphAddrContext*, bool*)                                                                : 5377091  : +1.72%  : 4.12%  : +0.0268%
Compiler::optLocalAssertionIsEqualOrNotEqual(Compiler::optOp1Kind, unsigned int, Compiler::optOp2Kind, long, unsigned long* const&) : 1507878  : +16.46% : 1.16%  : +0.0075%
Compiler::fgAssertionGen(GenTree*)                                                                                                  : 1198394  : NA      : 0.92%  : +0.0060%
Compiler::fgKillDependentAssertions(unsigned int)                                                                                   : 671692   : +22.10% : 0.52%  : +0.0034%
GenTree::DefinesLocal(Compiler*, GenTreeLclVarCommon**, bool*, long*, unsigned int*)                                                : 404001   : +0.61%  : 0.31%  : +0.0020%
Compiler::optAssertionProp(unsigned long* const&, GenTree*, Statement*, BasicBlock*)                                                : 343019   : +0.46%  : 0.26%  : +0.0017%
Compiler::fgKillDependentAssertionsSingle(unsigned int)                                                                             : 316623   : +10.81% : 0.24%  : +0.0016%
Compiler::optAssertionIsNonNullInternal(GenTree*, unsigned long* const&)                                                            : 255071   : +1.88%  : 0.20%  : +0.0013%
Compiler::AssertionDsc::Equals(Compiler::AssertionDsc*, bool)                                                                       : 146167   : +0.39%  : 0.11%  : +0.0007%
Compiler::optGetAssertion(unsigned short)                                                                                           : -309470  : -4.31%  : 0.24%  : -0.0015%
Compiler::optAssertionProp_LclVar(unsigned long* const&, GenTreeLclVarCommon*, Statement*)                                          : -331995  : -0.50%  : 0.25%  : -0.0017%
Compiler::optAssertionInit(bool)                                                                                                    : -378584  : -6.98%  : 0.29%  : -0.0019%
Compiler::optAssertionGen(GenTree*)                                                                                                 : -562997  : -0.53%  : 0.43%  : -0.0028%
Compiler::GetAssertionDep(unsigned int)                                                                                             : -731197  : -2.90%  : 0.56%  : -0.0036%
Compiler::optAssertionReset(unsigned short)                                                                                         : -974712  : -8.89%  : 0.75%  : -0.0049%
Compiler::optAssertionIsSubrange(GenTree*, IntegralRange, unsigned long* const&)                                                    : -1347642 : -17.08% : 1.03%  : -0.0067%
Compiler::fgMorphBlocks()                                                                                                           : -2362170 : -40.42% : 1.81%  : -0.0118%
Compiler::fgMorphLeafLocal(GenTreeLclVarCommon*)                                                                                    : -3832890 : -11.77% : 2.94%  : -0.0191%
Compiler::fgMorphTree(GenTree*, Compiler::MorphAddrContext*)                                                                        : -8915512 : -4.80%  : 6.84%  : -0.0445%

Looks like Clang at least stopped inlining fgMorphTreeDone somewhere.

@AndyAyersMS
Copy link
Member Author

Yeah fgMorphTreeDone is obviously called a lot... likely with PGO it gets inlined once more.

@AndyAyersMS AndyAyersMS merged commit 3f70252 into dotnet:main Nov 3, 2023
126 of 129 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 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.

3 participants