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

Disallow IND<struct> except as a source of STORE_DYN_BLK #74784

Merged
merged 10 commits into from
Dec 15, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Aug 29, 2022

Allowing for the removal of some OBJ(ADDR(FIELD)) wrapping in impNormStructVal and others.

The diffs are expected to be positive: we now allow FIELD<struct> substitution into struct args. There are a small number of regressions in CG collections that arise due to the fact unused R2R-affected FIELD call arguments interact less favorably with HIR DCE than OBJ(ADDR(FIELD)): the latter would be reduced to ADDR(FIELD), which would then null-check the object directly instead of expanding into the NULLCHECK(ADD(obj, IND(...)) tree that the compiler cannot remove.

@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 Aug 29, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 29, 2022
@ghost
Copy link

ghost commented Aug 29, 2022

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

Issue Details

Namely, the code which was wrapping temps created for calls. The only case remaining is the "original" wrapping, it will take a bit more work to get rid of it without regressions.

Also perform a bit of miscellaneous code cleanup.

We're expecting mostly positive diffs, with some regressions from the common sources:

  1. More inlining due to fewer temps.
  2. Interactions with forward substitution, copy propagation, CSE, etc.
Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion force-pushed the impNormStructVal branch 2 times, most recently from 0df0121 to ea85b39 Compare September 23, 2022 17:11
@SingleAccretion SingleAccretion force-pushed the impNormStructVal branch 6 times, most recently from e307c88 to 0b9acb3 Compare November 10, 2022 13:45
@SingleAccretion SingleAccretion changed the title Delete a bit more OBJ(ADDR(...)) wrapping from impNormStructVal Diagnosing failures Nov 10, 2022
@SingleAccretion SingleAccretion force-pushed the impNormStructVal branch 3 times, most recently from 6bb7f02 to 0fa4c9c Compare December 3, 2022 14:39
@SingleAccretion SingleAccretion marked this pull request as ready for review December 3, 2022 19:08
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch jakobbotsch merged commit dad39c7 into dotnet:main Dec 15, 2022
@jakobbotsch
Copy link
Member

Thanks!

@SingleAccretion SingleAccretion deleted the impNormStructVal branch December 15, 2022 17:09
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants