Fix assertions related to combining `cmn (extended-register)` (#113337) #113376

snickolls-arm · 2025-03-11T14:55:47Z

Fixes issues highlighted by Fuzzlyn, related to erroneously combining cast=>negate=>compare into cmn (extended-register) when the second operand is an integral constant.

Fixes: #113337

…#113337) Fixes issues highlighted by Fuzzlyn, related to erroneously combining cast=>negate=>compare into `cmn (extended-register)` when the second operand is an integral constant.

jakobbotsch · 2025-03-11T15:07:44Z

src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs

+using System.Runtime.CompilerServices;
+using Xunit;
+
+public class Program


Suggested change

public class Program

public class Runtime_113337

Also, add the standard header (.NET Foundation)

jakobbotsch · 2025-03-11T15:07:59Z

src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs

+    public static int TestEntryPoint()
+    {
+        // Checking for successful compilation
+        issue1();
+        issue2();
+        return 100;
+    }


Suggested change

public static int TestEntryPoint()

{

// Checking for successful compilation

issue1();

issue2();

return 100;

}

public static void TestEntryPoint()

{

// Checking for successful compilation

issue1();

issue2();

}

jakobbotsch · 2025-03-11T15:08:57Z

src/coreclr/jit/lowerarmarch.cpp

@@ -404,9 +404,9 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN
                return false;
            }

-            if (childNode->gtGetOp1()->OperIs(GT_CAST))
+            if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->IsIntegralConst())


Suggested change

if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->IsIntegralConst())

if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->isContained())

Should this check rather be like this? Uncontained constant will still be in a register, and that should be fine.

This line change doesn't seem to work but I'm not sure why just yet, I'll follow up shortly. It's already guaranteed that the constant is not contained here, because there is a check at the beginning of the function that will return if one of the nodes is contained already.

I've also noticed that gtGetOp2 isn't a reliable way of getting the other operand, as childNode could be op1 or op2, so I've fixed this.

Indeed seems like something else must be wrong. An uncontained constant is, like any other node, going to be in a register, so this check should not make a difference if we know it is not contained.

Looking again, I think it was down to the cast node needing more restrictions. I thought the assertion isGeneralRegister(reg2) was firing on REG_NA, but it was actually firing on a floating point register. The other assertion is down to containing a cast node that already contains a LCL_VAR, this shouldn't be allowed.

…g casts that already contain some memory operation

jakobbotsch · 2025-03-13T20:11:12Z

src/coreclr/jit/lowerarmarch.cpp

+                }
+
+                // Cannot contain the cast if it may contain or is already containing a memory operation.
+                if (IsContainableMemoryOp(castOp) || castOp->isContained())


Is the IsContainableMemoryOp check necessary?

Assuming castOp and cast have already been analyzed for containment before this, then it's not necessary. Is that always true?

Yes, that is always true. castOp is an operand, and operands always have to appear earlier in linear order.

jakobbotsch · 2025-03-24T16:36:44Z

/azp run Fuzzlyn

azure-pipelines · 2025-03-24T16:36:58Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2025-03-24T19:26:23Z

Are the Fuzzlyn found "unreached" assertion failures a different problem?

E.g.

// Generated by Fuzzlyn v2.5 on 2025-03-24 18:20:14
// Run on Arm64 Linux
// Seed: 8261718896722533474-vectort,vector64,vector128,armadvsimd,armadvsimdarm64,armaes,armarmbase,armarmbasearm64,armcrc32,armcrc32arm64,armdp,armrdm,armrdmarm64,armsha1,armsha256,armsve
// Reduced from 58.9 KiB to 0.5 KiB in 00:00:22
// Hits JIT assert in Release:
// Assertion failed 'unreached' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Generate code' (IL size 72; hash 0xade6b36b; FullOpts)
// 
//     File: /__w/1/s/src/coreclr/jit/codegenarm64.cpp Line: 3530
// 
using System;
using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;

public class Program
{
    public static Vector<ulong>[] s_3;
    public static void Main()
    {
        var vr3 = Vector.Create<short>(1);
        var vr4 = (short)0;
        var vr5 = Vector128.CreateScalar(vr4).AsVector();
        if ((Sve.TestFirstTrue(vr3, vr5) | (3268100580U != (-(uint)Sve.SaturatingDecrementBy16BitElementCount(0, 1)))))
        {
            s_3[0] = s_3[0];
        }
    }
}

snickolls-arm · 2025-03-25T09:41:22Z

Are the Fuzzlyn found "unreached" assertion failures a different problem?

I haven't seen this assertion fire before, but it looks like it's related to this change in containment patterns. I'll take a look today.

snickolls-arm · 2025-03-25T14:57:16Z

I haven't seen this assertion fire before, but it looks like it's related to this change in containment patterns. I'll take a look today.

@jakobbotsch

This new assertion is fired when a NEG node contains a CAST node. I think this situation is caused by the CCMP transform that is running on the code generated by Fuzzlyn. On this line it clears the containment relation between CMP => NEG after bashing the CMP node to CCMP, but it is not clearing the containment relation between NEG => CAST.

It's a slightly different problem but still introduced by the same change as before. Would you prefer this fix as a separate pull request, or a continuation here?

jakobbotsch · 2025-03-25T15:06:49Z

I wonder if we should rethink this transformation, to make it more robust. Would it make more sense to introduce a new GT_CMPNEG node to represent the "compare negated" version of the instruction? The multiple levels of containments always seem to cause trouble (it's not the first time we see issues like this).

jakobbotsch · 2025-03-25T16:24:32Z

Alternatively I would be ok with skipping the transformation in TryLowerAndOrToCCMP when an operand is a contained non-constant. That's an easier fix.

snickolls-arm · 2025-03-25T16:39:40Z

I wonder if we should rethink this transformation, to make it more robust. Would it make more sense to introduce a new GT_CMPNEG node to represent the "compare negated" version of the instruction? The multiple levels of containments always seem to cause trouble (it's not the first time we see issues like this).

This could work. There's potentially some complications around the precedence of this transform over the CCMP transform, as the node would need to be split up again when converted into CCMP after the fact.

Alternatively I would be ok with skipping the transformation in TryLowerAndOrToCCMP when an operand is a contained non-constant. That's an easier fix.

OK, I'll try this next.

jakobbotsch · 2025-03-25T16:43:44Z

This could work. There's potentially some complications around the precedence of this transform over the CCMP transform, as the node would need to be split up again when converted into CCMP after the fact.

Yeah, but very possibly we would just skip the conversion into ccmp for these cases, unless we can show the missed benefit to be significant. The key is that things would be correct by construction (a missed optimization is much better than asserts or potential silent bad codegen).

snickolls-arm · 2025-03-26T16:02:30Z

Are the Fuzzlyn found "unreached" assertion failures a different problem?

E.g.

// Generated by Fuzzlyn v2.5 on 2025-03-24 18:20:14
// Run on Arm64 Linux
// Seed: 8261718896722533474-vectort,vector64,vector128,armadvsimd,armadvsimdarm64,armaes,armarmbase,armarmbasearm64,armcrc32,armcrc32arm64,armdp,armrdm,armrdmarm64,armsha1,armsha256,armsve
// Reduced from 58.9 KiB to 0.5 KiB in 00:00:22
// Hits JIT assert in Release:
// Assertion failed 'unreached' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Generate code' (IL size 72; hash 0xade6b36b; FullOpts)
// 
//     File: /__w/1/s/src/coreclr/jit/codegenarm64.cpp Line: 3530
// 
using System;
using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;

public class Program
{
    public static Vector<ulong>[] s_3;
    public static void Main()
    {
        var vr3 = Vector.Create<short>(1);
        var vr4 = (short)0;
        var vr5 = Vector128.CreateScalar(vr4).AsVector();
        if ((Sve.TestFirstTrue(vr3, vr5) | (3268100580U != (-(uint)Sve.SaturatingDecrementBy16BitElementCount(0, 1)))))
        {
            s_3[0] = s_3[0];
        }
    }
}

These latest build failures seem to be network related as far as I understand. I've added this case to the test class and it seems to be compiling now.

jakobbotsch · 2025-03-26T16:07:59Z

/azp run Fuzzlyn

azure-pipelines · 2025-03-26T16:08:14Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2025-03-26T19:13:31Z

Fuzzlyn failures are #113923, #113939, #113940. Linux-x64 failures are azurelinux timeouts.

jakobbotsch · 2025-03-26T19:13:49Z

/ba-g Azurelinux timeouts

jakobbotsch · 2025-03-26T19:14:51Z

src/coreclr/jit/lowerarmarch.cpp

@@ -3187,11 +3200,15 @@ bool Lowering::TryLowerAndOrToCCMP(GenTreeOp* tree, GenTree** next)
    //
    GenCondition cond1;
    if (op2->OperIsCmpCompare() && varTypeIsIntegralOrI(op2->gtGetOp1()) && IsInvariantInRange(op2, tree) &&
+        (op2->gtGetOp1()->IsIntegralConst() || !op2->gtGetOp1()->isContained()) &&
+        (op2->gtGetOp2() == nullptr || op2->gtGetOp2()->IsIntegralConst() || !op2->gtGetOp2()->isContained()) &&


This nullptr check is unnecessary (we already checked for CmpCompare above, all of those are binops). But let's not wait for another round of CI here.

jakobbotsch · 2025-03-26T19:18:16Z

There's a small number of regressions like this one:

Can you take a look at why, and potentially submit a follow-up if it's something unexpected?

I'm going to merge this to clean up Fuzzlyn runs.

Fix assertions related to combining cmn (extended-register) (dotnet…

12755c1

…#113337) Fixes issues highlighted by Fuzzlyn, related to erroneously combining cast=>negate=>compare into `cmn (extended-register)` when the second operand is an integral constant.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 11, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 11, 2025

snickolls-arm mentioned this pull request Mar 11, 2025

JIT: Assertion failed 'isGeneralRegister(reg2)' during 'Generate code' #113337

Closed

jakobbotsch reviewed Mar 11, 2025

View reviewed changes

Fix test and address code review comments

4e6d417

snickolls-arm added 3 commits March 12, 2025 10:26

Fix tests for other architectures

ee62a3a

Restrict containment to integer->integer casts, and prevent containin…

1f900f0

…g casts that already contain some memory operation

Formatting

6079bb4

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

Assert failure '!CREATE_CHECK_STRING(bSmallObjectHeapPtr || bLargeObjectHeapPtr)' #111922

Open

jakobbotsch reviewed Mar 13, 2025

View reviewed changes

snickolls-arm and others added 2 commits March 24, 2025 14:06

Merge branch 'main' into fix-113337

4e51d8b

Remove check for IsContainableMemoryOp

7bcb67c

This was referenced Mar 24, 2025

System.Net.Quic tests timeout #107761

Open

Segmentation fault running System.Security.Cryptography.Tests #113785

Closed

Fix interaction with CCMP optimization

83ebe28

This was referenced Mar 26, 2025

[QUIC & HTTP/3] Handshake Timeout on tests #104426

Open

System.OperationCanceledException : The operation was canceled. dotnet/dnceng#5278

Open

System.Net.Requests test timeout #113883

Closed

jakobbotsch reviewed Mar 26, 2025

View reviewed changes

jakobbotsch approved these changes Mar 26, 2025

View reviewed changes

jakobbotsch merged commit 4c6e4e2 into dotnet:main Mar 26, 2025
118 of 128 checks passed

github-actions bot locked and limited conversation to collaborators Apr 26, 2025

	if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->IsIntegralConst())
	if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->isContained())

Fix assertions related to combining cmn (extended-register) (#113337) #113376

Fix assertions related to combining cmn (extended-register) (#113337) #113376

Uh oh!

Conversation

snickolls-arm commented Mar 11, 2025 • edited by kunalspathak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Mar 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Mar 24, 2025

Uh oh!

azure-pipelines bot commented Mar 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

snickolls-arm commented Mar 25, 2025

Uh oh!

snickolls-arm commented Mar 25, 2025

Uh oh!

jakobbotsch commented Mar 25, 2025

Uh oh!

jakobbotsch commented Mar 25, 2025

Uh oh!

snickolls-arm commented Mar 25, 2025

Uh oh!

jakobbotsch commented Mar 25, 2025

Uh oh!

snickolls-arm commented Mar 26, 2025

Uh oh!

jakobbotsch commented Mar 26, 2025

Uh oh!

azure-pipelines bot commented Mar 26, 2025

Uh oh!

jakobbotsch commented Mar 26, 2025

Uh oh!

jakobbotsch commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Mar 26, 2025

Uh oh!

Uh oh!

Uh oh!

Fix assertions related to combining `cmn (extended-register)` (#113337) #113376

Fix assertions related to combining `cmn (extended-register)` (#113337) #113376

snickolls-arm commented Mar 11, 2025 •

edited by kunalspathak

Loading

jakobbotsch Mar 11, 2025 •

edited

Loading

jakobbotsch commented Mar 24, 2025 •

edited

Loading