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

Introduce FIELD_ADDR and use it for TLS statics and instance class fields #77353

Merged
merged 16 commits into from
Nov 10, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Oct 23, 2022

Removing two uses of GT_ADDR in the process.

We expect a couple diffs in tests where we now fail to hoist a vector constant because we don't resolve struct handles (thus recognize SIMD types) for FIELD_ADDRs, as well as one case of new substitution due to smaller tree size. We also expect a minor TP improvement in the optimized case.

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

ghost commented Oct 23, 2022

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

Issue Details

Removing two uses of GT_ADDR in the process.

We expect a couple diffs in tests where we now fail to hoist a vector constant because we don't resolve struct handles (thus recognize SIMD types) for FIELD_ADDRs. We also expect a minor TP improvement.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion changed the title Introduce FIELD_ADDR and use it for TLS statics and class instance fields Introduce FIELD_ADDR and use it for TLS statics and instance class fields Oct 23, 2022
@SingleAccretion SingleAccretion force-pushed the Field-Addr-Upstream branch 4 times, most recently from e7d5f78 to d22eede Compare October 23, 2022 17:51
@SingleAccretion SingleAccretion force-pushed the Field-Addr-Upstream branch 3 times, most recently from 71a410f to a872f5a Compare October 23, 2022 21:22
With NativeAOT, we can have FIELD_ADDR nodes that are
effectively NOPs (those for the method table pointer field).

This means not all operators that the expansion produces
will be simple (in fact, they can be more or less arbitrary).

This means we cannot simply call "fgMorphSmpOp" as we used to.
Unfortunately, we cannot just call "fgMorphTree" either, because
it propagates assertions on newly created (or, I suppose, bashed)
IND nodes. This creates lots of new cases where GTF_ORDER_SIDEEFF
is applied to these nodes, affecting CQ.

Work around this by calling "fgMorphTree" for non-simple operators
only.
@SingleAccretion SingleAccretion marked this pull request as ready for review October 24, 2022 20:35
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@BruceForstall
Copy link
Member

Diffs

@build-analysis build-analysis bot mentioned this pull request Nov 4, 2022
2 tasks
@jakobbotsch
Copy link
Member

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch
Copy link
Member

The unreduced BadResult failures in the Fuzzlyn run looks like a known intermittent failure that I have seen before, I don't think it is related to this PR.

// Return Value:
// The expanded tree - a GT_ADD.
//
GenTree* Compiler::fgMorphExpandTlsFieldAddr(GenTree* tree)
Copy link
Member

Choose a reason for hiding this comment

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

I assume there's no actual changes here except constructing the address part of this? The diff is quite hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one change: tlsRef = gtNewIndir(TYP_I_IMPL, tlsRef, GTF_IND_NONFAULTING | GTF_IND_INVARIANT); - for the indir off of the TLS handle.

(Not doing this was making fgDebugCheckFlags fail)

@jakobbotsch
Copy link
Member

The stress failures were #78023

@jakobbotsch jakobbotsch merged commit 78322ae into dotnet:main Nov 10, 2022
@SingleAccretion SingleAccretion deleted the Field-Addr-Upstream branch November 11, 2022 14:14
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2022
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.

3 participants