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

arm64: Optimise GT/GE/LT/LE comparisons in return statements #112863

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jonathandavies-arm
Copy link
Contributor

@jonathandavies-arm jonathandavies-arm commented Feb 24, 2025

@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 Feb 24, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 24, 2025
@jonathandavies-arm
Copy link
Contributor Author

@a74nh @kunalspathak @dotnet/arm64-contrib

if (cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag() &&
BlockRange().TryGetUse(cmp, &use))
if (cmp->OperIs(GT_EQ, GT_NE, GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) &&
op1->SupportsSettingZeroFlag() && BlockRange().TryGetUse(cmp, &use))
Copy link
Member

Choose a reason for hiding this comment

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

SupportsSettingZeroFlag should be renamed to something that indicates the set of flags needed by these comparisons.

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.

Can you fix the test errors in CI?

if (cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag() &&
BlockRange().TryGetUse(cmp, &use))
if (cmp->OperIs(GT_EQ, GT_NE, GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) &&
op1->SupportsSettingFlags() && BlockRange().TryGetUse(cmp, &use))
Copy link
Member

Choose a reason for hiding this comment

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

should this be arm64 specific change? seems there are some test failures on x64 as well.

Copy link
Member

Choose a reason for hiding this comment

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

Why make this arm64 specific? We should figure out why this would not work for x64 and what would be different between x64 and arm64 in those cases before we just disable this.

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to have this focused change for arm64. We should have separate PR if we want to enable it for x64.

Copy link
Member

@jakobbotsch jakobbotsch Mar 4, 2025

Choose a reason for hiding this comment

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

That's ok with me -- in that case I think we should introduce a new SupportsSettingResultFlags that returns false on all platforms but arm64, and then make the check something like

(cmp->OperIs(GT_EQ, GT_NE) && op1->SupportsSettingZeroFlag()) || (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op1->SupportsSettingResultFlags())

(The rename to SupportsSettingFlags does not look right for xarch, since that function currently queries emitter::DoesWriteZeroFlag for hardware intrinsics. That might be the cause of the failures.)

@@ -5,13 +5,13 @@
using System.Runtime.CompilerServices;
using Xunit;

namespace TestBitwiseClearShift
namespace TestBic
Copy link
Member

Choose a reason for hiding this comment

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

Also need to update corresponding .csproj file to reflect the new file name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kunalspathak
Copy link
Member

Were you able to find out the other failing tests?

Change-Id: I0dbd2de796a11badde063684761cec93fc1c8355
* Update SupportsSettingResultFlags() comments

Change-Id: I6c1f9ad86933329575442ce212b322650ac71b34
Change-Id: I76753a9b034934aa0f1c765854421ce9e7b970d3
@jakobbotsch
Copy link
Member

Thanks, looks good to me now. Can you also update the title of the PR to be accurate?

@jonathandavies-arm jonathandavies-arm changed the title arm64: Set zero flag for more comparisons arm64: Optimise GT/GE/LT/LE comparisons in return statements Mar 5, 2025
// The backend expects any node for which the flags will be consumed to be
// marked with GTF_SET_FLAGS.
//
bool GenTree::SupportsSettingResultFlags()
Copy link
Member

Choose a reason for hiding this comment

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

The build is failing because this was already defined above

@kunalspathak
Copy link
Member

@jonathandavies-arm - did you verify the latest CI failures?

@jonathandavies-arm
Copy link
Contributor Author

@jonathandavies-arm - did you verify the latest CI failures?
Sorry I've been away, I am now back looking at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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