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

Liveness fix for struct enreg. #51851

Merged
merged 3 commits into from
Apr 28, 2021
Merged

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Apr 26, 2021

Delete some unnecessary mustInit. The change is required for a small struct enreg change.

spmi changes:

benchmarks.run.windows.x64.checked.mch
Total bytes of delta: -725 (-1.10% of base)
71 total files with Code Size differences (69 improved, 2 regressed), 0 unchanged.

libraries.crossgen.windows.x64.checked
Total bytes of delta: -479 (-0.37% of base)
42 total files with Code Size differences (41 improved, 1 regressed), 0 unchanged.

libraries.crossgen2.windows.x64.checked.mch
Total bytes of delta: -3296 (-0.90% of base)
373 total files with Code Size differences (345 improved, 28 regressed), 8 unchanged.

libraries.pmi.windows.x64.checked
Total bytes of delta: -2101 (-0.62% of base)
243 total files with Code Size differences (214 improved, 29 regressed), 0 unchanged.

tests.pmi.windows.x64.checked
Total bytes of delta: -18637 (-0.81% of base)
1475 total files with Code Size differences (1472 improved, 3 regressed), 2 unchanged.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 26, 2021
@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@sandreenko sandreenko added the enhancement Product code improvement that does NOT require public API changes/additions label Apr 27, 2021
@sandreenko sandreenko self-assigned this Apr 27, 2021
@sandreenko sandreenko marked this pull request as ready for review April 27, 2021 08:03
// Make sure that we use uncontained address nodes only for variables
// that will be marked as mustInit and will be alive throught the whole block even when tracked.
assert(lclVarAddr->isContained() || !varDsc->lvTracked || varTypeIsStruct(varDsc));
// TODO: support this assert for uses, see https://github.com/dotnet/runtime/issues/51900.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dotnet dotnet deleted a comment from azure-pipelines bot Apr 27, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Apr 27, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Apr 27, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Apr 27, 2021
@sandreenko
Copy link
Contributor Author

There are many failures but they all look unrelated, the same gcstress failures happened in https://dev.azure.com/dnceng/public/_build/results?buildId=1095005&view=ms.vss-test-web.build-test-results-tab, the jitstress failure presented in the previous run as well.

@sandreenko
Copy link
Contributor Author

PTAL @BruceForstall @dotnet/jit-contrib , cc @erozenfeld


if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) && varDsc->lvOnFrame)
Copy link
Member

@erozenfeld erozenfeld Apr 27, 2021

Choose a reason for hiding this comment

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

The new logic doesn't take into account varDsc->lvOnFrame. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, if I add an assert like if (mustInitThisVar) assert(varDsc->lvOnFrame); it won't fail (x64 spmi) but it is a current implicit contract so we probably should not rely on it.

if (hasGCPtr && !isTracked) -> not tracked means not in register -> lvOnFrame == true
else if (hasGCPtr && isStruct) -> do not enregister structs, with https://github.com/dotnet/runtime/pull/51850/checks?check_run_id=2447290001 won't enregister structs with GC pointers -> lvOnFrame == true
if (!isTracked)  -> not tracked means not in register -> lvOnFrame == true

I will return the condition, thank you.

src/coreclr/jit/lower.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegencommon.cpp Outdated Show resolved Hide resolved
@sandreenko sandreenko merged commit 973f1eb into dotnet:main Apr 28, 2021
@sandreenko sandreenko deleted the testLiveness branch April 28, 2021 19:15
@sandreenko sandreenko mentioned this pull request May 10, 2021
10 tasks
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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 enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants