Skip to content

[Arm64] Replace pairs of str with stp#85032

Merged
kunalspathak merged 20 commits intodotnet:mainfrom
kunalspathak:stp
Apr 27, 2023
Merged

[Arm64] Replace pairs of str with stp#85032
kunalspathak merged 20 commits intodotnet:mainfrom
kunalspathak:stp

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented Apr 19, 2023

Optimize pair of str with stp along with tracking the gc information.

Contributes to #55365 and #77010
Fixes: #35134, #35133 and #81278

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

ghost commented Apr 19, 2023

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

Issue Details

Optimize pair of str with stp along with tracking the gc information.

Contributes to #55365 and #77010
Fixes: #35134, #35133 and #81278

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 19, 2023

Diffs looks good (as expected):

image

I will run gcstress job to make sure there are no gc holes.

@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 19, 2023

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

azure-pipelines bot commented Apr 19, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 20, 2023

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

azure-pipelines bot commented Apr 20, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 20, 2023

gcstress is green with few known failures.

@kunalspathak kunalspathak marked this pull request as ready for review April 20, 2023 16:40
@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 20, 2023

@dotnet/jit-contrib , @BruceForstall

cc: @a74nh @SwapnilGaikwad

@BruceForstall
Copy link
Contributor

BruceForstall commented Apr 21, 2023

Diffs

@BruceForstall
Copy link
Contributor

BruceForstall commented Apr 21, 2023 •
edited
Loading

This diffs look great. Interesting there are some zero-size diffs where we convert str/stp to stp/str.

fwiw, I noticed a case maybe we could do better in libraries.crossgen2 on win-arm64:

16194.dasm - System.Reflection.TypeNameParser:GetType(System.String,bool,bool,System.Reflection.Assembly):System.Type
...
             strb    wzr, [fp, #0x38]
             strb    wzr, [fp, #0x39]
             strb    wzr, [fp, #0x3A]
             strb    wzr, [fp, #0x3B]
             strb    wzr, [fp, #0x3C]

=>

             str     wzr, [fp, #0x38]
             strb    wzr, [fp, #0x3C]

?

BruceForstall
BruceForstall suggested changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Can you show any diffs (including GC diffs) where both addresses are GC lcl vars?

src/coreclr/jit/emitarm64.cpp Outdated
(immediate).
*/

void emitter::emitIns_SS_R_R_R_I(instruction ins,
Copy link
Contributor

@BruceForstall BruceForstall Apr 21, 2023

Choose a reason for hiding this comment

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

Why is this not a variant or extension of emitIns_S_S_R_R?

In that case, no reg3 is passed because it is determined based on the lclvar. In this case, there are 2 lclvar. Presumably they would both have the same FPbased value, thus determining the same address base register. In fact, you could probably determine / assert the offsets are correct.

Copy link
Contributor Author

@kunalspathak kunalspathak Apr 21, 2023

Choose a reason for hiding this comment

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

I have referred the code from emitIns_R_R_R_I() instead which operates on vector registers as well. For example, it operates on scale of 2, 3 and 4, whereas emitIns_S_S_R_R() operates just on scale == 3. Also, it has special handling for useRegForAdr which I assume is just for the stack stores. And yes, the reg3 is always FP or SP.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 21, 2023
@BruceForstall
Copy link
Contributor

BruceForstall commented Apr 21, 2023

I presume the MinOpts TP regression is due to extra checks related to instrDesc size, and possibly due to fewer constants fitting in a small instrDesc.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 21, 2023
@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 21, 2023

Can you show any diffs (including GC diffs) where both addresses are GC lcl vars?

Sure, this is one example 4997 in coreclr_test.mch of windows.arm64 where I missed the gc tracking, and fixed it in 92b67de.

Here is the diff. Of course, we need to fix #84504
image

@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 21, 2023

I presume the MinOpts TP regression is due to extra checks related to instrDesc size, and possibly due to fewer constants fitting in a small instrDesc.

correct.

@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 21, 2023

fwiw, I noticed a case maybe we could do better in libraries.crossgen2 on win-arm64:

That is an interesting pattern coming from accessing 4~5 bool fields that are next to each other.

runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameParser.CoreCLR.cs

Lines 20 to 24 in abaad8f

private bool _throwOnError;
private bool _ignoreCase;
private bool _extensibleParser;
private bool _requireAssemblyQualifiedName;
private bool _suppressContextualReflectionContext;

Unfortunately, the popular peephole optimization that we do today just check the last instruction. We could use the infrastructure that we have recently developed to see last N instructions and combine them, but honestly, I don't know if removing last N instructions is well tested and works. I would expect this optimization to happen higher up in morph or something, but should be tracked as a separate issue, although I am not sure how many of such patterns exist.

BruceForstall
BruceForstall suggested changes Apr 21, 2023
View reviewed changes
src/coreclr/jit/emitarm64.cpp Outdated
regNumber reg3,
ssize_t imm,
int varx1,
int offs,
Copy link
Contributor

@BruceForstall BruceForstall Apr 21, 2023

Choose a reason for hiding this comment

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

offs here is the offset within a local var. Since there are 2 lclvars, there should be two offsets: offs1, offs2.

Copy link
Contributor Author

@kunalspathak kunalspathak Apr 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The 2nd offset would be just offs + TARGET_POINTER_SIZE so we don't need to pass it.

Copy link
Contributor

@BruceForstall BruceForstall Apr 21, 2023

Choose a reason for hiding this comment

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

That's not correct. The offset is within the local, not from the base register.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 21, 2023
@build-analysis build-analysis bot mentioned this pull request Apr 25, 2023
Open
2 tasks
@build-analysis build-analysis bot mentioned this pull request Apr 25, 2023
Closed
@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 25, 2023

@BruceForstall - this should be ready to review again.

@build-analysis build-analysis bot mentioned this pull request Apr 26, 2023
Closed
a74nh
a74nh reviewed Apr 26, 2023
View reviewed changes
a74nh
a74nh reviewed Apr 26, 2023
View reviewed changes
@kunalspathak
Copy link
Contributor Author

kunalspathak commented Apr 26, 2023

Latest diffs:

image

Seems like some good TPdiffs because of consolidation of ldp and stp instructions:

image

BruceForstall
BruceForstall suggested changes Apr 26, 2023
View reviewed changes
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 26, 2023
@kunalspathak kunalspathak merged commit 318186b into dotnet:main Apr 27, 2023
@kunalspathak kunalspathak deleted the stp branch April 27, 2023 06:10
This was referenced Apr 27, 2023
Closed
Closed
Closed
@EgorBo EgorBo mentioned this pull request May 4, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@a74nh a74nh a74nh left review comments

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ARM64: Optimize pair of "str reg, [fp]" to stp

3 participants

@kunalspathak @BruceForstall @a74nh