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.

Sign up for GitHub

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

Jump to bottom

Add optimisation of incrementing SELECT at lowering on arm64. #91262

Merged
merged 2 commits into from
Sep 17, 2023
Merged

Add optimisation of incrementing SELECT at lowering on arm64. #91262

merged 2 commits into from
Sep 17, 2023

Conversation

c272
Copy link
Contributor

@c272 c272 commented Aug 29, 2023

This patch adds support for optimising SELECT or SELECT_CC nodes which contain a child of type GT_ADD with a second operand constant value of 1 (increment) to be optimised into a SELECT_INC or SELECT_INCCC node. This results in an instruction saving on arm64 down to a single csinc.

SPMI diffs for this patch (performed on win-arm64 since SPMI is currently broken on linux-arm64 as per #91257):
https://gist.github.com/c272/449a2760c1897f74722600bf200712b2

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

ghost commented Aug 29, 2023

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

Issue Details

This patch adds support for optimising SELECT or SELECT_CC nodes which contain a child of type GT_ADD with a second operand constant value of 1 (increment) to be optimised into a SELECT_INC or SELECT_INCCC node. This results in an instruction saving on arm64 down to a single csinc.

SPMI diffs for this patch (performed on win-arm64 since SPMI is currently broken on linux-arm64 as per #91257):
https://gist.github.com/c272/449a2760c1897f74722600bf200712b2

Author: c272
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@c272
Copy link
Contributor Author

c272 commented Aug 29, 2023

Unsure why the CLA bot is being noisy, although the check seems to have passed anyway.
I've previously submitted a patch, so I don't think this should be an issue?

@a74nh
Copy link
Contributor

a74nh commented Aug 29, 2023

This is going to catch x = (condition) ? x+1 : y; cases, which the previous CINC patch didn't do. We should have added that at the same time as the CNEG and CINV changes.

Would have expected more spmidiff differences, but the diffs found look good.

@build-analysis build-analysis bot mentioned this pull request Aug 29, 2023
Closed
@c272 c272 force-pushed the selectinc-patch branch 5 times, most recently from c24a2a0 to a5943bd Compare August 30, 2023 12:54
@build-analysis build-analysis bot mentioned this pull request Aug 30, 2023
Closed
@c272 c272 force-pushed the selectinc-patch branch 3 times, most recently from 73ec0a0 to 57bb8d9 Compare August 31, 2023 10:38
@c272
Add optimisation of incrementing SELECT at lowering on arm64.
83adbd6 
This patch adds support for optimising SELECT or SELECT_CC nodes
which contain a child of type GT_ADD with a second operand constant
value of 1 (increment) to be optimised into a SELECT_INC or
SELECT_INCCC node.

Change-Id: Ia26da6f4c0e3a75143e133964cbb76243d0822de
@c272 c272 force-pushed the selectinc-patch branch from 57bb8d9 to 83adbd6 Compare August 31, 2023 15:58
@build-analysis build-analysis bot mentioned this pull request Aug 31, 2023
Closed
@kunalspathak
Copy link
Member

@c272 - could you please resolve the build errors?

@c272
Copy link
Contributor Author

c272 commented Sep 12, 2023 •
edited
Loading

@kunalspathak Merged in latest from main to trigger a rebuild. Don't think the failures from before were due to the patch, but the logs were already cleaned. The libraries test failure for linux_musl here also seems unrelated.

kunalspathak
kunalspathak approved these changes Sep 17, 2023
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contributions.

@kunalspathak
Copy link
Member

Failures are unrelated.

@kunalspathak kunalspathak merged commit 1972683 into dotnet:main Sep 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@c272 @a74nh @kunalspathak