Skip to content

Use a proper check rather than an assert #103995

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

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Use a proper check rather than an assert #103995

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 12 additions & 6 deletions src/coreclr/jit/hwintrinsicxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,18 +267,24 @@ static CORINFO_InstructionSet lookupInstructionSet(const char* className)
{
if (strncmp(className + 6, "128", 3) == 0)
{
assert((className[9] == '\0') || (strcmp(className + 9, "`1") == 0));
return InstructionSet_Vector128;
if ((className[9] == '\0') || (strcmp(className + 9, "`1") == 0))
Copy link
Member Author

@tannergooding tannergooding Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The need to do this seems like a bug in NAOT, Vector512DebugView is not marked as intrinsic and correspondingly neither should the synthesized Boxed_Vector512DebugView that's being created for NAOT.

The fact that something in NAOT is marking it as such means that its going down the impIntrinsic path when it shouldn't be and that correspondingly is failing the assert here. It could potentially lead to other undesirable downstream behavior if arbitrary types are being marked as [Intrinsic] when they shouldn't

CC. @MichalStrehovsky

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jun 25, 2024

Choose a reason for hiding this comment

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

The issue is that jit is asking questions about the unboxing entrypoint here. We prefix them with Boxed_. We can probably drop the prefix, it will only make debugging the compiler a bit more confusing.

CoreCLR VM is probably reporting these still as intrinsic with the original name, but they are all sorts of weird.

Copy link
Member Author

@tannergooding tannergooding Jun 25, 2024

Choose a reason for hiding this comment

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

The coreclr vm should only report types marked as intrinsic as intrinsic, it’s fairly explicit about that handling today to ensure that not anything named Vector128 is treated as such

Copy link
Member Author

@tannergooding tannergooding Jun 25, 2024

Choose a reason for hiding this comment

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

It’s not clear what would be causing it to go down this path in naot otherwise, this path is only accessible from lookupId which is only done for impIntrinsic, which should only be invoked for methods marked as Intrinsic

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jun 25, 2024

Choose a reason for hiding this comment

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

The coreclr vm should only report types marked as intrinsic as intrinsic, it’s fairly explicit about that handling today to ensure that not anything named Vector128 is treated as such

It is Vector128, but we got to it though something like a devirtualization. It's an entrypoint of the valuetype method that accepts a boxed this instead of the typical ref to this. The JIT typically converts these to a non-unboxing/normal entrypoint if it was able to elide the box.

Copy link
Member Author

@tannergooding tannergooding Jun 25, 2024

Choose a reason for hiding this comment

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

👍, not sure what would be triggering this then given how the VM marks methods as intrinsic

I might try to debug it further later tonight

Copy link
Member Author

@tannergooding tannergooding Jun 26, 2024

Choose a reason for hiding this comment

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

Ok, so this is flowing down through:

 	clrjit_win_x64_x64.dll!assertAbort(const char * why, const char * file, unsigned int line) Line 288	C++
 	clrjit_win_x64_x64.dll!lookupInstructionSet(const char * className) Line 280	C++
 	clrjit_win_x64_x64.dll!HWIntrinsicInfo::lookupIsa(const char * className, const char * enclosingClassName) Line 321	C++
 	clrjit_win_x64_x64.dll!HWIntrinsicInfo::lookupId(Compiler * comp, CORINFO_SIG_INFO * sig, const char * className, const char * methodName, const char * enclosingClassName) Line 707	C++
 	clrjit_win_x64_x64.dll!Compiler::lookupNamedIntrinsic(CORINFO_METHOD_STRUCT_ * method) Line 10418	C++
>	clrjit_win_x64_x64.dll!Compiler::fgFindJumpTargets<1>(const unsigned char * codeAddr, unsigned int codeSize, FixedBitVect * jumpTarget) Line 1314	C++

Which determines isIntrinsic via isIntrinsic = eeIsIntrinsic(methodHnd);, which in turn ends up querying the information from ValueTypeInstanceMethodWithHiddenParameter.IsIntrinsic which unconditionally sets IsIntrinsic to true: https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.BoxedTypes.cs#L606

So this is indeed the AOT VM setting things as Intrinsic, CC. @MichalStrehovsky

Is that expected or should this rather be false since it is a NoInlining+InternalCall and doesn't appear to have any JIT side handling that would necessitate it being intrinsic?

Copy link
Member Author

@tannergooding tannergooding Jun 26, 2024

Choose a reason for hiding this comment

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

-- It looks like DefaultInterfaceMethodImplementationWithHiddenParameter.IsINtrinsic is doing the same thing: https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.InterfaceThunks.cs#L290

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jun 26, 2024

Choose a reason for hiding this comment

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

Is that expected or should this rather be false since it is a NoInlining+InternalCall and doesn't appear to have any JIT side handling that would necessitate it being intrinsic?

We rely on this being intrinsic elsewhere in the compiler.

I looked at how unboxing MethodDesc are constructed in the CoreCLR VM and I believe they inherit everything from the non-unboxing flavor, from token (that decides the name), to IsIntrinsic bit (see CreateMethodDesc). PR to do the equivalent at #104008.

Copy link
Member

@jkotas jkotas Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is that expected or should this rather be false since it is a NoInlining+InternalCall and doesn't appear to have any JIT side handling that would necessitate it being intrinsic?

IsIntrinsic tells the JIT to check the type and method name to see whether it is something the JIT is interested in handling, but it does not say that the JIT must handle it.

The JIT is expected to handle every potential CoreLib method being reported as intrinsic. It will be slow because it will have to check the name every time, but it should work.

{
return InstructionSet_Vector128;
}
}
else if (strncmp(className + 6, "256", 3) == 0)
{
assert((className[9] == '\0') || (strcmp(className + 9, "`1") == 0));
return InstructionSet_Vector256;
if ((className[9] == '\0') || (strcmp(className + 9, "`1") == 0))
{
return InstructionSet_Vector256;
}
}
else if (strncmp(className + 6, "512", 3) == 0)
{
assert((className[9] == '\0') || (strcmp(className + 9, "`1") == 0));
return InstructionSet_Vector512;
if ((className[9] == '\0') || (strcmp(className + 9, "`1") == 0))
{
return InstructionSet_Vector512;
}
}
}
else if (strcmp(className, "VL") == 0)
Expand Down