Skip to content

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented Sep 2, 2021

When we undo the struct promotion, we revert all the promoted fields except in the code path where we would return the promoted field. We were keeping the promoted field leading to bad codegen.

No asmdiff on libraries/benchmarks/coreclr_test.

Since the original repro needed jitstress switch, I verified that this PR fixes the code we geenrate.

Fixes: #57912

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 2, 2021
@ghost
Copy link

ghost commented Sep 2, 2021

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

Issue Details

When we undo the struct promotion, we revert all the promoted fields except in the code path where we would return the promoted field. We were keeping the promoted field leading to bad codegen.

Fixes: #57912

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Contributor Author

@dotnet/jit-contrib

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Changes look good. I assume fgMorphRetInd is relatively new?

Is there any way to add checking for cases where we mess up this undo?

@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Sep 2, 2021
@kunalspathak
Copy link
Contributor Author

Changes look good. I assume fgMorphRetInd is relatively new?

It was added last year in #37745

Is there any way to add checking for cases where we mess up this undo?

I tried looking around and I didn't see a good place to have this check. What I ended up doing is introducing a field lvUndoneStructPromotion and then, during mark local reference, whenever we see that we are increasing the reference count of a struct field, we make sure that its parent struct didn't have lvUndoneStructPromotion set.

By running superpmi replay on coreclr, it hits the newly added assert on exactly same tests.

@kunalspathak kunalspathak merged commit 19eba6b into dotnet:main Sep 3, 2021
@kunalspathak
Copy link
Contributor Author

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1196839095

@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] seq_short_1_d fails

4 participants