Skip to content

Remove GT_ADDEX and replace with more generalized containment handling - #76273

Merged
tannergooding merged 10 commits into
dotnet:mainfrom
tannergooding:fix-68028
Sep 30, 2022
Merged

Remove GT_ADDEX and replace with more generalized containment handling#76273
tannergooding merged 10 commits into
dotnet:mainfrom
tannergooding:fix-68028

Conversation

@tannergooding

Copy link
Copy Markdown
Member

This makes more progress towards #68028

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 27, 2022
@ghost

Copy link
Copy Markdown

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

Issue Details

This makes more progress towards #68028

Author:tannergooding
Assignees:tannergooding
Labels:

area-CodeGen-coreclr

Milestone:-

@tannergooding
tannergooding marked this pull request as draft September 27, 2022 21:25
@tannergooding
tannergooding marked this pull request as ready for review September 28, 2022 03:51
@tannergooding

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@EgorBo

Copy link
Copy Markdown
Member

@tannergooding

tannergooding commented Sep 28, 2022

Copy link
Copy Markdown
MemberAuthor

Any idea what causes size regressions? https://dev.azure.com/dnceng-public/public/_build/results?buildId=32858&view=ms.vss-build-web.run-extensions-tab

@EgorBo, looks like some places aren't getting contained anymore and they were on the previous path.

Looking again, this would be because the original path handles GT_CAST having a contained op itself by clearing the containment

Where-as the new path checked the containment the CastOp and exited early if it was contained.

@tannergooding

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@TIHanTIHan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar to what we did for msub and madd - let containment drive all of it rather than have a separate op.

@tannergooding

Copy link
Copy Markdown
MemberAuthor
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run.Linux.arm64.checked.mch16,583,652-988
coreclr_tests.run.Linux.arm64.checked.mch164,421,868-9,380
libraries.crossgen2.Linux.arm64.checked.mch23,302,248-244
libraries.pmi.Linux.arm64.checked.mch64,553,072-2,312
libraries_tests.pmi.Linux.arm64.checked.mch143,626,968-1,568

MinOpts is a size regression since it now only does this opt when optimizations are enabled, but that also gives a very minor throughput improvement (-0.01%).

If we were willing to take a regression to MinOpts instead (+0.01%) and do this containment even in minopts, then we'd save more than 24k size instead. Given that Arm64 has fixed-sized instructions, this might be beneficial overall.

Comment threadsrc/coreclr/jit/lowerarmarch.cpp
Comment threadsrc/coreclr/jit/lowerarmarch.cpp
@tannergooding

Copy link
Copy Markdown
MemberAuthor

Many diffs are cases like the following:

- add x0, x0, w26, UXTW- ldrb w0, [x0]+ ldrb w0, [x0, w26, UXTW #2]

These seem to mostly be due to ADDEX not being properly handled in many places while ADD always is.


There are a few small regressions such as:

+ lsl w0, w0, #8
ldr x1, [fp, #0xA8]	// [V192 tmp170]
ldrb w1, [x1, #0x01]
- add w1, w1, w0, LSL #8+ add w1, w0, w1

Notably we're checking if childNode->gtGetOp1() (the value to be shifted) is contained, the original doesn't (but it shouldn't be anyways since we need a register for it).

We're also checking IsSafeToContainMem(parentNode, childNode) where-as the original wasn't. I expect this is the "actual" reason for the "larger diff" since we always do "strict" checking and we don't allow reordering across anything with a "side effect". That being said, there is no actual interdependence between these two and so it is technically "safe" anyways.

Comment threadsrc/coreclr/jit/lowerarmarch.cpp
@tannergooding

Copy link
Copy Markdown
MemberAuthor

/azp run Fuzzlyn

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@EgorBoEgorBo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice diffs from a clean up 🙂

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

4 participants

@tannergooding@EgorBo@TIHan@SingleAccretion