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

Take advantage of simplifications enabled by folding all indirect access to locals #79722

Merged
merged 10 commits into from
Mar 31, 2023

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Dec 15, 2022

We delete code which needed to deal with the fact tracked locals in HIR could be accesses indirectly.

Diffs are primarily from the reintroduced morph folding. Sources:

  1. Late morph in non-null assertion prop does not set GTF_ORDER_SIDEEFF on the local field (as it would on an indirection), allowing for some optOptimizeBools action.
  2. CSE is less aggressive because LCL_FLD is cheaper than IND(LCL_VAR_ADDR).
  3. Args sorting is a bit different for the same reason (source of zero-sized diffs).

There is also a nice TP win, 0.3% - 0.5% reduction in instruction retired under FullOpts.

Note: diff better viewed with the "ignore whitespace changes" option.

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

ghost commented Dec 15, 2022

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

Issue Details

We delete code which needed to deal with the fact tracked locals in HIR could be accesses indirectly.

Diffs: a few regressions in tests where we are more conservative with address-exposed locals accessed via IND(LCL_VAR_ADDR) (either due to volatile loads/stores, or BLK/OBJ-based access with some forward substitution in-between local morph and global morph). One case in libraries where the GS reordering kicks in for two more locals because they are assigned directly via LCL_FLD (GS didn't handle indirect stores).

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion changed the title Take advantage of simplifications enabled by folding all indirect all access to locals Take advantage of simplifications enabled by folding all indirect access to locals Dec 15, 2022
SingleAccretion added a commit to SingleAccretion/runtime that referenced this pull request Jan 4, 2023
Good on me for noticing it a few weeks ago in dotnet#79722...

```
// Otherwise, must be local lhs form. TODO-Bug: this doesn't account for LCL_FLD.
// This will miss memory havoc induced by address-exposed local field stores.
```
@ghost ghost closed this Feb 10, 2023
@ghost
Copy link

ghost commented Feb 10, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost closed this Mar 12, 2023
@ghost
Copy link

ghost commented Mar 12, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch jakobbotsch self-requested a review March 30, 2023 13:55
@SingleAccretion SingleAccretion mentioned this pull request Mar 30, 2023
Comment on lines -243 to -254
GenTreeLclVarCommon* lclVarTree = nullptr;
GenTree* addrArg = tree->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true);
if (!addrArg->DefinesLocalAddr(&lclVarTree))
{
fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed);
}
else
{
// Defines a local addr
assert(lclVarTree != nullptr);
fgMarkUseDef(lclVarTree->AsLclVarCommon());
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member

Failure is known according to build analysis.

@jakobbotsch jakobbotsch merged commit 9744bf9 into dotnet:main Mar 31, 2023
@SingleAccretion SingleAccretion deleted the DefinesLocal-Simplification branch March 31, 2023 16:38
@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 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