-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Improvements for dead store removal. #38004
Conversation
030a585
to
ed7a888
Compare
ed7a888
to
a5bcc78
Compare
1a7a5be
to
3d8ed62
Compare
Framework x64 pmi diffs:
Benchmarks x64 pmi diffs:
|
@CarolEidt @sandreenko @AndyAyersMS PTAL cc: @dotnet/jit-contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I suspect that getting some of these conditions right was tricky! Have you measured the throughput or size impact of adding the defsInBlock
map?
FYI - the timeout on the |
3d8ed62
to
de300c5
Compare
Yes, this took multiple iterations. Tracking down failures caused by removing stores to pinned locals with ref count 1 was painful.
I measured crossgen of SPC with pin-icount. Somewhat surprisingly, it shows a throughput improvement of 0.02%, which is close to noise level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, do you want to run outerloop/stress tests before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
Yes, will rebase and run outerloop and stress pipelines overnight. |
1. Don't mark fields of dependently promoted structs as untracked. 2. Remove some stores whose lhs local has a ref count of 1 when running late liveness. We can rely on ref counts since they are calculated right before the late liveness pass. 3. Remove dead GT_STORE_BLK in addition to GT_STOREIND in the late liveness pass. 4. Remove dead stores to untracked locals in the late liveness pass. 5. Allow optRemoveRedundantZeroInits to remove some redundant initializations of tracked locals. Move the phase to right after liveness so that SSA is correct after removing assignments to tracked locals.
de300c5
to
5d02113
Compare
Noted, and thanks for highlighting it to us. I'll check some telemetry, but the likely cause is that this queue is very busy. We'll continue to monitor the pressure. |
Loader/binding/assemblies/assemblybugs/37910/Ii/Ii.sh failures in outerloop and jitstress2-jitstressregs pipelines are #38452. JIT\jit64\mcc\interop\mcc_i7*\mcc_i7*.cmd failures in jitstress2-jitstressregs are #36199. All failures in gcstress0x3-gcstress0xc pipeline are also seen in master except crossgen2smoke: https://dev.azure.com/dnceng/public/_build/results?buildId=705744&view=ms.vss-test-web.build-test-results-tab&runId=21861154&resultId=109360&paneView=debug |
Failure mode in crossgen2smoke is very similar to #34316. |
I was able to repro crossgen2smoke failure locally both with my changes (on the 6th run) and without my changes (on the 7th run). I opened #38482. |
@garath when you see logging like |
Improvements for dead store removal.
Don't mark fields of dependently promoted structs as untracked.
Remove some stores whose lhs local has a ref count of 1 when running
late liveness. We can rely on ref counts since they are calculated right
before the late liveness pass.
Remove dead GT_STORE_BLK in addition to GT_STOREIND in the late
liveness pass.
Remove dead stores to untracked locals in the late liveness pass.
Allow optRemoveRedundantZeroInits to remove some redundant initializations
of tracked locals. Move the phase to right after liveness so that SSA is correct
after removing assignments to tracked locals.