Optimize out metadata for typeof(X).IsValueType and .IsEnum#118528
Optimize out metadata for typeof(X).IsValueType and .IsEnum#118528MichalStrehovsky wants to merge 3 commits intodotnet:mainfrom
typeof(X).IsValueType and .IsEnum#118528Conversation
We don't need reflection metadata to answer this. Change salvaged out of dotnet#118479, rt-sz will decide if we want it.
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the AOT compilation process by avoiding the generation of unnecessary reflection metadata for simple type queries. The optimization specifically targets patterns like typeof(X).IsValueType and typeof(X).IsEnum where the answer can be determined at compile time without requiring full reflection metadata.
Key changes:
- Adds pattern detection for
typeof(X).IsValueTypeandtypeof(X).IsEnumcall sequences - Modifies helper ID assignment to use
NecessaryTypeHandleinstead of full metadata when these patterns are detected
src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs
Outdated
Show resolved
Hide resolved
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
|
The savings are borderline, but maybe worth it given the low complexity: Size statisticsPull request #118528
|
…r.cs Co-authored-by: Copilot <[email protected]>
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| { | ||
| if (reader.HasNext | ||
| && reader.ReadILOpcode() is ILOpcode.callvirt or ILOpcode.call | ||
| && _methodIL.GetObject(reader.ReadILToken()) is MethodDesc { Name: "get_IsValueType" or "get_IsEnum"}) |
There was a problem hiding this comment.
Add IsPrimitive too? It is used in some places in BCL.
There was a problem hiding this comment.
Let's see what rt-sz says. We can do this for several other properties but I wasn't sure how much of a coupling between the compiler and the BCL we are okay with in this area.
There was a problem hiding this comment.
If we wanted to avoid coupling between the compiler and the BCL, we would make this pattern must-expand in the JIT. It would reduce the coupling to JIT/scanner that exists in many places.
There was a problem hiding this comment.
Do we have a test coverage for the (rare) situation when the JIT does not expand this pattern as an intrinsic?
There was a problem hiding this comment.
If we wanted to avoid coupling between the compiler and the BCL, we would make this pattern must-expand in the JIT. It would reduce the coupling to JIT/scanner that exists in many places.
Hmm, I haven't even though about it in the context of the JIT optimization.
My original reason for why we don't need the metadata is because the implementation in RuntimeType will just go straight to the MethodTable if there is one and ask there. But yeah, RyuJIT will expand it before that, so we won't even get there.
I guess the only way to ensure this codepath really works would be to run unoptimized mode with the scanner enabled. I don't really know if I want to add a test pass with that. Or make it MustExpand in RyuJIT but not sure if that would be okay (cc @EgorBo).
There was a problem hiding this comment.
Not sure I understand how e.g. IsPrimitive can be a must-expand intrinsic -- ho do we expand it for bool Foo(Type t) => t.IsPrimitive; ?
There was a problem hiding this comment.
It would have to be a special "must expand if this comes from a typeof" rule (this is the logic that scanner is doing.)
There was a problem hiding this comment.
I guess that will work, but it kind of relies that JIT doesn't spill typeof to a locally anyhow (e.g. under some stress)
There was a problem hiding this comment.
Here is the patch to do that:
From b9ee418054231419e5cbd0e7a8f8da1a329758d5 Mon Sep 17 00:00:00 2001
From: EgorBo <[email protected]>
Date: Tue, 12 Aug 2025 16:55:12 +0200
Subject: [PATCH] make typeof().IsX must expand
---
src/coreclr/jit/importercalls.cpp | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp
index a4e2e454b96..431668fc403 100644
--- a/src/coreclr/jit/importercalls.cpp
+++ b/src/coreclr/jit/importercalls.cpp
@@ -3500,6 +3500,19 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd,
mustExpand = true;
break;
+ case NI_System_Type_get_IsEnum:
+ case NI_System_Type_get_IsValueType:
+ case NI_System_Type_get_IsByRefLike:
+ case NI_System_Type_get_IsPrimitive:
+ {
+ CORINFO_CLASS_HANDLE hClass = NO_CLASS_HANDLE;
+ if (gtIsTypeof(impStackTop().val, &hClass))
+ {
+ mustExpand = true;
+ }
+ }
+ break;
+
case NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference:
mustExpand |= sig->sigInst.methInstCount == 1;
break;
--
2.45.2.windows.1
There was a problem hiding this comment.
Yeah, if this gets sketchy, we can just come back to #118528 (comment): "but maybe worth it given the low complexity" - if the complexity is in ensuring test coverage in corner cases, this makes it no longer "low complexity" and maybe not worth it.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Closing since we can't reliably test the fallbacks. |
We don't need reflection metadata to answer this.
Change salvaged out of #118479, rt-sz will decide if we want it.