-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add a VN-based dead store removal phase #77990
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis new phase will iterate over the stores references through SSA descriptors, and delete those which do not change the local's value, determined via VN's selection mechanism. TP impact:
|
670cca0
to
84ea3ab
Compare
Fewer regressions; a bit faster TP-wise. Diffs (libraries.pmi) for the full implementation: --------------------------------------------------------------------------------- 2,236 contexts with diffs (1,871 improvements, 111 regressions) -25,583/+1,467 bytes Diffs (libraries.pmi) for partial definitions only: --------------------------------------------------------------------------------- 1,687 contexts with diffs (1,683 improvements, 3 regressions) -23,661/+25 bytes TP impact about the same, ~0.05% against ~0.045%.
This reverts commit 127dccf.
As one would suspect, we're bumping here into bugs with how simplistic is VN's logic for whether something will be zero-initialized in the prolog is. Will take some iterations to sort this out. Edit: done. |
(The "lvaIsOSRLocal" check in codegen was redundant)
84ea3ab
to
d6547a3
Compare
@dotnet/jit-contrib (Edit: naturally, we should stress and fuzz this) |
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
Azure Pipelines successfully started running 3 pipeline(s). |
Not sure what happened in the win-arm64 Fuzzlyn run, the artifacts were apparently not published in the expected location. I checked the partitions manually and saw no errors. |
Looks like there are stress failures that will need to be investigated.
|
The System.Drawing tests are a clear case of silent bad codegen. Unfortunately, without an ARM64 device, it is not trivial to understand where exactly the failure comes from. The pattern of tests failures suggests the problem is somewhere in the auto-generated interop code, and it is of a very particular nature (most test results are 2x of what they should be). Will try PMIing the assembly manually next. Edit: notably, the tests in question only fail with TC on... Edit: the local debugging plan didn't pan out. Will have to use the CI to get more information. Edit: more CI debugging did not reproduce the issue. Curious. |
@jakobbotsch Let's run another libraries stress here. I've run the System.Drawing tests 3 times in the supposedly same configuration in
So, if we see it fail again, it was most likely I have also pushed a debug-only commit to capture the dump if we do see it fail. |
/azp run runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 1 pipeline(s). |
I'll see if I can repro the failure on win-arm64 some time on the weekend or next week. |
No luck, the stress was clean. Will revert the debug commit. |
This reverts commit 797dc55.
I was not able to repro the failures after running the exact bits that failed in CI on my win-arm64 machine in a loop for a while, so I think it can be ignored (maybe some kind of GDI bug?). |
Thank you for looking! In the meantime I found a reference to the second failure here: #72365 (comment). |
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay |
Azure Pipelines successfully started running 2 pipeline(s). |
Awesome work, thank you! |
Improvements on win-x64: dotnet/perf-autofiling-issues#10064 |
This new phase will iterate over the stores referenced through SSA descriptors, and delete those which do not change the local's value, determined via VN's selection mechanism.
TP:
~0.1%
.SPMI diffs (Win-x64): -5K for
benchmarks.run
, -24K forlibraries.pmi
.Overall minor impact on both counts, but the amount of code is not large too.
Detailed TP regression breakdown:
The impact of the zero-init fix is more than I'd like, but it is not clear if it can be ameliorated without compromising correctness; the logic is not trivial.
The impact of fetching
ZeroObj
VNs is also somewhat surprising; one more reason to rework the "init block" IR shape as it exists today to have somethingSTRUCT
-typed on the RHS.All that said, the improvements from #77655 should cover for this, with a bit left still.