arm64: Optimise GT/GE/LT/LE comparisons in return statements by jonathandavies-arm · Pull Request #112863 · dotnet/runtime

jonathandavies-arm · 2025-02-24T14:50:44Z

Contributes to Review the multi-op instruction usage for Arm64 #68028

jonathandavies-arm · 2025-02-24T14:51:31Z

@a74nh @kunalspathak @dotnet/arm64-contrib

jakobbotsch · 2025-02-24T23:24:28Z

src/coreclr/jit/lower.cpp

-    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))


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

kunalspathak

Can you fix the test errors in CI?

kunalspathak · 2025-02-28T02:22:14Z

src/coreclr/jit/lower.cpp

-    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))


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

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.

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

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.)

kunalspathak · 2025-02-28T02:22:45Z

src/tests/JIT/opt/InstructionCombining/Bic.cs

 using Xunit;

-namespace TestBitwiseClearShift
+namespace TestBic


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

kunalspathak · 2025-03-04T18:03:44Z

Were you able to find out the other failing tests?

Change-Id: I0dbd2de796a11badde063684761cec93fc1c8355

src/coreclr/jit/gentree.cpp

* Update SupportsSettingResultFlags() comments Change-Id: I6c1f9ad86933329575442ce212b322650ac71b34

Change-Id: I76753a9b034934aa0f1c765854421ce9e7b970d3

jakobbotsch · 2025-03-05T10:51:51Z

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

jakobbotsch · 2025-03-06T10:57:19Z

src/coreclr/jit/gentree.cpp

+//    The backend expects any node for which the flags will be consumed to be
+//    marked with GTF_SET_FLAGS.
+//
+bool GenTree::SupportsSettingResultFlags()


The build is failing because this was already defined above

Change-Id: I18402694c3a8b63f71b9cb2b810a0d4db05f4aaf

kunalspathak · 2025-03-11T16:21:45Z

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

jonathandavies-arm · 2025-03-18T07:59:49Z

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

jonathandavies-arm · 2025-03-19T14:20:38Z

Hi @kunalspathak , I'm having trouble running the test that is failing in the pipeline. Please can you see if what I'm trying in correct?

I'm trying to run the System.Diagnostics.StackTrace.Tests locally in order to investigate the current failure.
I've build the tests using ./dotnet.sh build src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj -c Release but I've having trouble running them.

I've tried:

./dotnet.sh run src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj -c Release
./artifacts/bin/testhost/net10.0-linux-Release-x64/shared/Microsoft.NETCore.App/10.0.0/corerun ./artifacts/bin/System.Diagnostics.StackTrace.Tests/Release/net10.0/System.Diagnostics.StackTrace.Tests.dll
cd into ./artifacts/bin/System.Diagnostics.StackTrace.Tests/Release/net10.0 and run ./RunTests.sh -r ~/projects/dotnet/dotnet where ~/projects/dotnet/dotnet contains .NET 10.0 SDK

The last one seems most like how the tests are run on the GitHub build. I get this error when I use RunTests.sh

$ ./RunTests.sh -r ~/projects/dotnet/dotnet
========================= Begin custom configuration settings ==============================
export __IsXUnitLogCheckerSupported=1
export XUNIT_HIDE_PASSING_OUTPUT_DIAGNOSTICS=1
========================== End custom configuration settings ===============================
----- start Wed Mar 19 02:15:00 PM GMT 2025 =============== To repro directly: =====================================================
pushd .
/home/jondav01/projects/dotnet/dotnet/dotnet exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json /home/jondav01/.nuget/packages/microsoft.dotnet.xunitconsolerunner/2.9.2-beta.25125.2/build/../tools/net/xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================
~/projects/dotnet/runtime/artifacts/bin/System.Diagnostics.StackTrace.Tests/Release/net10.0 ~/projects/dotnet/runtime/artifacts/bin/System.Diagnostics.StackTrace.Tests/Release/net10.0
You must install or update .NET to run this application.

App: /home/jondav01/.nuget/packages/microsoft.dotnet.xunitconsolerunner/2.9.2-beta.25125.2/tools/net/xunit.console.dll
Architecture: x64
Framework: 'Microsoft.NETCore.App', version '10.0.0' (x64)
.NET location: /home/jondav01/projects/dotnet/dotnet/

The following frameworks were found:
  10.0.0-preview.2.25163.2 at [/home/jondav01/projects/dotnet/dotnet/shared/Microsoft.NETCore.App]

Learn more:
https://aka.ms/dotnet/app-launch-failed

To install missing framework, download:
https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=10.0.0&arch=x64&rid=linux-x64&os=ubuntu.24.04
~/projects/dotnet/runtime/artifacts/bin/System.Diagnostics.StackTrace.Tests/Release/net10.0
----- end Wed Mar 19 02:15:00 PM GMT 2025 ----- exit code 150 ----------------------------------------------------------
ulimit -c value: 0
cat /proc/sys/kernel/core_pattern: |/usr/share/apport/apport -p%p -s%s -c%c -d%d -P%P -u%u -g%g -- %E
cat /proc/sys/kernel/core_uses_pid: 0
cat: /proc/sys/kernel/coredump_filter: No such file or directory
cat /proc/sys/kernel/coredump_filter:
Looking around for any Linux dumps...

I'm running this on an x64 machine and will run it on an arm64 once I can run this test.

kunalspathak · 2025-03-19T17:47:24Z

Hi @kunalspathak , I'm having trouble running the test that is failing in the pipeline

let me sync-up with you offline.

…p-gt

dotnet-policy-service · 2025-05-28T04:00:44Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

kunalspathak · 2025-05-29T15:17:04Z

We won't be working on it right away.

arm64: Set zero flag for more comparisons

e89057c

ghost 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 bot added the community-contribution Indicates that the PR has been added by a community member label Feb 24, 2025

jakobbotsch reviewed Feb 24, 2025

View reviewed changes

jonathandavies-arm added 2 commits February 25, 2025 10:52

Rename SupportsSettingZeroFlag to SupportsSettingFlags

077d8cd

Merge branch 'main' into upstream/ce/cmp-gt

ff70f1d

kunalspathak suggested changes Feb 28, 2025

View reviewed changes

Fix Bic csproj

f573c26

build-analysis bot mentioned this pull request Feb 28, 2025

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Closed

3 tasks

Only support other comparisons on ARM64

1f488a6

build-analysis bot mentioned this pull request Mar 4, 2025

ManagedToNativeGenerator task fails: Microsoft.Build.Framework.BuildException.GenericBuildTransferredException: Unsupported parameter type in method 'PInvokeTests.StructureTests+CdeclSimpleExplStructDelegate.Invoke' #113025

Open

Add SupportsSettingResultFlags() and restore SupportsSettingZeroFlag()

d507f31

Change-Id: I0dbd2de796a11badde063684761cec93fc1c8355

jakobbotsch reviewed Mar 5, 2025

View reviewed changes

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

jakobbotsch reviewed Mar 5, 2025

View reviewed changes

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

jonathandavies-arm added 2 commits March 5, 2025 10:35

* Call SupportsSettingResultFlags() from SupportsSettingZeroFlag()

24aa340

* Update SupportsSettingResultFlags() comments Change-Id: I6c1f9ad86933329575442ce212b322650ac71b34

Fix formatting

d42e598

Change-Id: I76753a9b034934aa0f1c765854421ce9e7b970d3

jakobbotsch approved these changes Mar 5, 2025

View reviewed changes

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

Merge branch 'main' into upstream/ce/cmp-gt

dc2e09f

jakobbotsch reviewed Mar 6, 2025

View reviewed changes

jonathandavies-arm added 2 commits March 6, 2025 12:43

Remove duplicated SupportsSettingResultFlags() definition

61c1144

Change-Id: I18402694c3a8b63f71b9cb2b810a0d4db05f4aaf

Merge branch 'main' into upstream/ce/cmp-gt

753e19f

Merge remote-tracking branch 'upstream-fork/main' into upstream/ce/cm…

fd77f15

…p-gt

This was referenced Mar 26, 2025

System.Net.Quic tests timeout #107761

Closed

System.TimeoutException : The operation has timed out. dotnet/dnceng#5279

Closed

System.Net.Requests test timeout #113883

Closed

jonathandavies-arm added 3 commits April 9, 2025 10:03

Merge branch 'main' into upstream/ce/cmp-gt

0134f3a

Merge branch 'main' into upstream/ce/cmp-gt

908b045

Only build stack frame if skipFrames is greater than zero

a17dece

build-analysis bot mentioned this pull request Apr 24, 2025

CI flakiness: mono interpreter build getting killed #114123

Open

jonathandavies-arm added 3 commits May 6, 2025 10:12

Merge branch 'main' into upstream/ce/cmp-gt

60c2cbb

Don't allow ADD/SUB because they don't work with the overflow

00f0b60

Merge branch 'main' into upstream/ce/cmp-gt

82b8f63

kunalspathak added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 13, 2025

dotnet-policy-service bot added the no-recent-activity label May 28, 2025

kunalspathak closed this May 29, 2025

github-actions bot locked and limited conversation to collaborators Jun 29, 2025

Conversation

jonathandavies-arm commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathandavies-arm commented Feb 24, 2025

Uh oh!

jakobbotsch Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

kunalspathak Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

kunalspathak Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalspathak Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

jonathandavies-arm Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented Mar 4, 2025

Uh oh!

Uh oh!

Uh oh!

jakobbotsch commented Mar 5, 2025

Uh oh!

jakobbotsch Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented Mar 11, 2025

Uh oh!

jonathandavies-arm commented Mar 18, 2025

Uh oh!

jonathandavies-arm commented Mar 19, 2025

Uh oh!

kunalspathak commented Mar 19, 2025

Uh oh!

dotnet-policy-service bot commented May 28, 2025

Uh oh!

kunalspathak commented May 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonathandavies-arm commented Feb 24, 2025 •

edited

Loading

jakobbotsch Mar 4, 2025 •

edited

Loading