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

Resolving an antigen failure #105260

Merged
merged 2 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30360,8 +30360,11 @@ GenTree* Compiler::gtFoldExprHWIntrinsic(GenTreeHWIntrinsic* tree)
#endif // !TARGET_XARCH && !TARGET_ARM64

DEBUG_DESTROY_NODE(op, tree);
INDEBUG(vectorNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

if (fgGlobalMorph)
{
INDEBUG(vectorNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
}
Comment on lines +30364 to +30367
Copy link
Member Author

Choose a reason for hiding this comment

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

The other INDEBUG(vectorNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); are properly under if (fgGlobalMorph) already

Copy link
Member

Choose a reason for hiding this comment

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

generally, it's enough to call fgMorphTreeDone(vectorNode) in these gtFoldExpr* apis - it will take care about fgGlobalMorph and the GTF_DEBUG_NODE_MORPHED flag (it also clears local assertionprop info, but that is important only if your function is called from morph and on an existing tree).

return vectorNode;
}
}
Expand Down
15 changes: 15 additions & 0 deletions src/coreclr/jit/lowerxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1570,6 +1570,21 @@ GenTree* Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node)
op2->SetUnusedValue();
}

// Since we have a double negation, it's possible that gtNext
// is op1 or user. If it is op1, then it's also possible the
// subsequent gtNext is user. We need to make sure to skip both
// in such a scenario since we're removing them.

if (nextNode == op1)
{
nextNode = nextNode->gtNext;
}

if (nextNode == user)
{
nextNode = nextNode->gtNext;
}
Comment on lines +1573 to +1586
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't my favorite fix, but given where we are in .NET 9 it's probably the safest one.

The TernaryLogic lowering being done by looking at the user is already a little different compared to other lowering logic, but it's how it got introduced several months back. More ideally we'd be handling this closer to how general containment gets handled, which is looking at the operands of a node instead.

That is, rather than saying "I am a BitwiseOper, is the node that consumes me also a BitwiseOper, we should probably be saying I am a BitwiseOper, are either of my operands also BitwiseOper`.

Such handling would avoid issues like this and also allow us to more easily take into account whether the ternarylogic usage is "profitable" (since it may not be if the two individual nodes could have been contained, as an example).

Copy link
Member Author

Choose a reason for hiding this comment

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

I notably wasn't able to get a simplified repro for this either, even running through the Antigen trimmer the smallest repro it found was 2100 lines of code.

Morph and other phases normally recognize/handle the double NOT much earlier and so its not typically found in practice.

Copy link
Member

Choose a reason for hiding this comment

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

I think I can understand the logic and the reason why you have to skip op1 and user for gtNext (I wonder if it's possible to just mark them as UnusedValue to keep them in LIR chain and continue LIR walk normally, but probably not).
And I agree that'd be much more natural to do this op while visiting the root XOR instead.


BlockRange().Remove(op3);
BlockRange().Remove(op1);
BlockRange().Remove(user);
Expand Down
36 changes: 36 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using Xunit;

#nullable disable

public class Runtime_105255_A
{
private static Vector512<uint> s_v512_uint_62 = Vector512<uint>.Zero;

public void Method0()

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime (Build coreclr Common Pri0 Test Build AnyOS AnyCPU checked)

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/__w/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime (Build linux-x64 Release AllSubsets_Mono_LLVMAot_RuntimeTests llvmaot)

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/__w/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime (Build linux-arm64 Release AllSubsets_Mono_Minijit_RuntimeTests minijit)

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/__w/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime (Build osx-x64 Release AllSubsets_Mono_Minijit_RuntimeTests minijit)

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/Users/runner/work/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime (Build osx-x64 Release AllSubsets_Mono_Interpreter_RuntimeTests monointerpreter)

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/Users/runner/work/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime (Build browser-wasm linux Release AllSubsets_Mono_RuntimeTests monointerpreter)

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/__w/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/__w/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]

Check failure on line 14 in src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs

View check run for this annotation

Azure Pipelines / runtime

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs#L14

src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.cs(14,17): error xUnit1013: Public method 'Method0' on test class 'Runtime_105255_A' should be marked as a Fact. Reduce the visibility of the method, or add a Fact attribute to the method. (https://xunit.net/xunit.analyzers/rules/xUnit1013) [/__w/1/s/src/tests/JIT/Regression/JitBlue/Runtime_105255/Runtime_105255.csproj]
{
s_v512_uint_62 = Vector512.LessThan<uint>(s_v512_uint_62, Vector512<uint>.Zero);
try
{
}
finally
{
for (int i = 0; i < 1; i++) ;
}
}

[Fact]
public static void TestEntryPoint() => new Runtime_105255_A().Method0();

/*
Assert failure(PID 5828 [0x000016c4], Thread: 6044 [0x179c]): Assertion failed '((tree->gtDebugFlags & GTF_DEBUG_NODE_MORPHED) == 0) && "ERROR: Already morphed this node!"' in 'TestClass:Method0():this' during 'Morph - Global' (IL size 22846; hash 0x46e9aa75; Tier0-FullOpts)
File: D:\a\_work\1\s\src\coreclr\jit\morph.cpp:12227
Image: C:\h\w\A715090A\p\CoreRoot\corerun.exe

Assertion failed '((tree->gtDebugFlags & GTF_DEBUG_NODE_MORPHED) == 0) && "ERROR: Already morphed this node!"' during 'Morph - Global'
*/
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
Loading