From 70570c201ba6b8fca829beceab3e1b1a4123a9c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Wed, 20 Mar 2024 22:16:36 +0100 Subject: [PATCH 1/7] Return type from exact method context for call returns --- src/coreclr/jit/fginline.cpp | 4 ++-- src/coreclr/jit/gentree.cpp | 20 ++++++++++++++++---- src/coreclr/jit/gentree.h | 6 +++--- src/coreclr/jit/importercalls.cpp | 16 ++++++++-------- src/coreclr/jit/inline.h | 6 +++--- 5 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 4c44bbc6bf33a8..ae163a2167d023 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -603,9 +603,9 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorIsTailPrefixedCall(); - if ((call->gtCallMoreFlags & GTF_CALL_M_HAS_LATE_DEVIRT_INFO) != 0) + if ((call->gtCallMoreFlags & GTF_CALL_M_HAS_EXACT_CONTEXT_INFO) != 0) { - context = call->gtLateDevirtualizationInfo->exactContextHnd; + context = call->gtExactContextInfo->exactContextHnd; // Note: we might call this multiple times for the same trees. // If the devirtualization below succeeds, the call becomes // non-virtual and we won't get here again. If it does not diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 90677f8f4975c1..0c99123965c887 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18548,12 +18548,24 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b } else if (call->gtCallType == CT_USER_FUNC) { - // For user calls, we can fetch the approximate return - // type info from the method handle. Unfortunately - // we've lost the exact context, so this is the best - // we can do for now. CORINFO_METHOD_HANDLE method = call->gtCallMethHnd; CORINFO_CLASS_HANDLE exactClass = nullptr; + if ((call->gtCallMoreFlags & GTF_CALL_M_HAS_EXACT_CONTEXT_INFO) != 0) + { + CORINFO_CONTEXT_HANDLE context = call->gtExactContextInfo->exactContextHnd; + + if (context != nullptr) + { + if (((SIZE_T)context & CORINFO_CONTEXTFLAGS_MASK) == CORINFO_CONTEXTFLAGS_CLASS) + { + exactClass = CORINFO_CLASS_HANDLE((SIZE_T)context & ~CORINFO_CONTEXTFLAGS_MASK); + } + else + { + method = CORINFO_METHOD_HANDLE((SIZE_T)context & ~CORINFO_CONTEXTFLAGS_MASK); + } + } + } CORINFO_SIG_INFO sig; eeGetMethodSig(method, &sig, exactClass); if (sig.retType == CORINFO_TYPE_VOID) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0d374e712d1b12..bac88fb2060cfa 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -174,7 +174,7 @@ struct BasicBlock; enum BasicBlockFlags : unsigned __int64; struct InlineCandidateInfo; struct HandleHistogramProfileCandidateInfo; -struct LateDevirtualizationInfo; +struct ExactContextInfo; typedef unsigned short AssertionIndex; @@ -4110,7 +4110,7 @@ enum GenTreeCallFlags : unsigned int GTF_CALL_M_ALLOC_SIDE_EFFECTS = 0x00100000, // this is a call to an allocator with side effects GTF_CALL_M_SUPPRESS_GC_TRANSITION = 0x00200000, // suppress the GC transition (i.e. during a pinvoke) but a separate GC safe point is required. GTF_CALL_M_EXPANDED_EARLY = 0x00800000, // the Virtual Call target address is expanded and placed in gtControlExpr in Morph rather than in Lower - GTF_CALL_M_HAS_LATE_DEVIRT_INFO = 0x01000000, // this call has late devirtualzation info + GTF_CALL_M_HAS_EXACT_CONTEXT_INFO = 0x01000000, // this call has exact context info GTF_CALL_M_LDVIRTFTN_INTERFACE = 0x02000000, // ldvirtftn on an interface type GTF_CALL_M_CAST_CAN_BE_EXPANDED = 0x04000000, // this cast (helper call) can be expanded if it's profitable. To be removed. GTF_CALL_M_CAST_OBJ_NONNULL = 0x08000000, // if we expand this specific cast we don't need to check the input object for null @@ -5644,7 +5644,7 @@ struct GenTreeCall final : public GenTree jitstd::vector* gtInlineCandidateInfoList; HandleHistogramProfileCandidateInfo* gtHandleHistogramProfileCandidateInfo; - LateDevirtualizationInfo* gtLateDevirtualizationInfo; + ExactContextInfo* gtExactContextInfo; CORINFO_GENERIC_HANDLE compileTimeHelperArgumentHandle; // Used to track type handle argument of dynamic helpers void* gtDirectCallAddress; // Used to pass direct call address between lower and codegen }; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index c9f852d94a0ad9..69105cf741baa4 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1384,20 +1384,20 @@ var_types Compiler::impImportCall(OPCODE opcode, } else { - // If the call is virtual, and has a generics context, and is not going to have a class probe, - // record the context for possible use during late devirt. + // If the call has a generic context, and is not going to have a class probe, + // record the context for possible future use in devirtualization. // // If we ever want to devirt at Tier0, and/or see issues where OSR methods under PGO lose // important devirtualizations, we'll want to allow both a class probe and a captured context. // - if (origCall->IsVirtual() && (origCall->gtCallType != CT_INDIRECT) && (exactContextHnd != nullptr) && + if ((origCall->gtCallType != CT_INDIRECT) && (exactContextHnd != nullptr) && (origCall->gtHandleHistogramProfileCandidateInfo == nullptr)) { JITDUMP("\nSaving context %p for call [%06u]\n", exactContextHnd, dspTreeID(origCall)); - origCall->gtCallMoreFlags |= GTF_CALL_M_HAS_LATE_DEVIRT_INFO; - LateDevirtualizationInfo* const info = new (this, CMK_Inlining) LateDevirtualizationInfo; - info->exactContextHnd = exactContextHnd; - origCall->gtLateDevirtualizationInfo = info; + origCall->gtCallMoreFlags |= GTF_CALL_M_HAS_EXACT_CONTEXT_INFO; + ExactContextInfo* const info = new (this, CMK_Inlining) ExactContextInfo; + info->exactContextHnd = exactContextHnd; + origCall->gtExactContextInfo = info; } if (isFatPointerCandidate) @@ -7764,7 +7764,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // it's a union field used for other things by virtual // stubs) call->ClearInlineInfo(); - call->gtCallMoreFlags &= ~GTF_CALL_M_HAS_LATE_DEVIRT_INFO; + call->gtCallMoreFlags &= ~GTF_CALL_M_HAS_EXACT_CONTEXT_INFO; #if defined(DEBUG) if (verbose) diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index 342dc3fca5d238..a1683181325c51 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -621,11 +621,11 @@ struct InlineCandidateInfo : public HandleHistogramProfileCandidateInfo InlineContext* inlinersContext; }; -// LateDevirtualizationInfo +// ExactContextInfo // -// Used to fill in missing contexts during late devirtualization. +// Used to fill in missing contexts during devirtualization. // -struct LateDevirtualizationInfo +struct ExactContextInfo { CORINFO_CONTEXT_HANDLE exactContextHnd; }; From 31f51211ffae5989191257714a32df175b15b9be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Thu, 21 Mar 2024 16:55:34 +0100 Subject: [PATCH 2/7] Improve NativeAOT assert messages --- .../tools/Common/JitInterface/CorInfoImpl.cs | 4 ++- .../ILCompiler.Compiler/Compiler/ILScanner.cs | 34 ++++++++++--------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 9366e1aa65b6d3..17772b3b52a445 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1199,8 +1199,10 @@ private void getMethodSig(CORINFO_METHOD_STRUCT_* ftn, CORINFO_SIG_INFO* sig, CO } else { - Debug.Assert(type.HasSameTypeDefinition(method.OwningType)); + Debug.Assert(type.HasSameTypeDefinition(method.OwningType), + $"{type.GetDisplayName()} has a different definition from {method.OwningType.GetDisplayName()}"); Instantiation methodInst = method.Instantiation; + Debug.Assert(type is InstantiatedType, $"{type.GetDisplayName()} is not a {nameof(InstantiatedType)}"); method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), (InstantiatedType)type); if (methodInst.Length > 0) { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs index 46e3df57efd1ff..6dd92215c2c629 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs @@ -282,25 +282,27 @@ internal override VTableSliceNode GetSlice(TypeDesc type) { // TODO: move ownership of compiler-generated entities to CompilerTypeSystemContext. // https://github.com/dotnet/corert/issues/3873 - if (type.GetTypeDefinition() is Internal.TypeSystem.Ecma.EcmaType) + if (type.GetTypeDefinition() is not Internal.TypeSystem.Ecma.EcmaType) + { + return new LazilyBuiltVTableSliceNode(type); + } + + if (_vtableSlices.TryGetValue(type, out IReadOnlyList slots)) { - if (!_vtableSlices.TryGetValue(type, out IReadOnlyList slots)) - { - // If we couldn't find the vtable slice information for this type, it's because the scanner - // didn't correctly predict what will be needed. - // To troubleshoot, compare the dependency graph of the scanner and the compiler. - // Follow the path from the node that requested this node to the root. - // On the path, you'll find a node that exists in both graphs, but it's predecessor - // only exists in the compiler's graph. That's the place to focus the investigation on. - // Use the ILCompiler-DependencyGraph-Viewer tool to investigate. - Debug.Assert(false); - string typeName = ExceptionTypeNameFormatter.Instance.FormatName(type); - throw new ScannerFailedException($"VTable of type '{typeName}' not computed by the IL scanner."); - } return new PrecomputedVTableSliceNode(type, slots); } - else - return new LazilyBuiltVTableSliceNode(type); + + // If we couldn't find the vtable slice information for this type, it's because the scanner + // didn't correctly predict what will be needed. + // To troubleshoot, compare the dependency graph of the scanner and the compiler. + // Follow the path from the node that requested this node to the root. + // On the path, you'll find a node that exists in both graphs, but it's predecessor + // only exists in the compiler's graph. That's the place to focus the investigation on. + // Use the ILCompiler-DependencyGraph-Viewer tool to investigate. + string typeName = ExceptionTypeNameFormatter.Instance.FormatName(type); + string message = $"VTable of type '{typeName}' not computed by the IL scanner."; + Debug.Fail(message); + throw new ScannerFailedException(message); } } From b0d91f288226100099a2e6bb60e0091313e87933 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Thu, 21 Mar 2024 17:19:54 +0100 Subject: [PATCH 3/7] Improve some more NAOT errors --- .../Compiler/RyuJitCompilation.cs | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs index 80aef8f4c2118a..e74527beb83676 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs @@ -193,6 +193,7 @@ private void CompileSingleMethod(CorInfoImpl corInfo, MethodCodeNode methodCodeN try { corInfo.CompileMethod(methodCodeNodeNeedingCode); + return; } catch (TypeSystemException ex) { @@ -200,23 +201,21 @@ private void CompileSingleMethod(CorInfoImpl corInfo, MethodCodeNode methodCodeN } } - if (exception != null) + if (exception is TypeSystemException.InvalidProgramException + && method.OwningType is MetadataType mdOwningType + && mdOwningType.HasCustomAttribute("System.Runtime.InteropServices", "ClassInterfaceAttribute")) { - // Try to compile the method again, but with a throwing method body this time. - MethodIL throwingIL = TypeSystemThrowingILEmitter.EmitIL(method, exception); - corInfo.CompileMethod(methodCodeNodeNeedingCode, throwingIL); - - if (exception is TypeSystemException.InvalidProgramException - && method.OwningType is MetadataType mdOwningType - && mdOwningType.HasCustomAttribute("System.Runtime.InteropServices", "ClassInterfaceAttribute")) - { - Logger.LogWarning(method, DiagnosticId.COMInteropNotSupportedInFullAOT); - } - if ((_compilationOptions & RyuJitCompilationOptions.UseResilience) != 0) - Logger.LogMessage($"Method '{method}' will always throw because: {exception.Message}"); - else - Logger.LogError($"Method will always throw because: {exception.Message}", 1005, method, MessageSubCategory.AotAnalysis); + Logger.LogWarning(method, DiagnosticId.COMInteropNotSupportedInFullAOT); } + + if ((_compilationOptions & RyuJitCompilationOptions.UseResilience) != 0) + Logger.LogMessage($"Method '{method}' will always throw because: {exception.Message}"); + else + Logger.LogError($"Method will always throw because: {exception.Message}", 1005, method, MessageSubCategory.AotAnalysis); + + // Try to compile the method again, but with a throwing method body this time. + MethodIL throwingIL = TypeSystemThrowingILEmitter.EmitIL(method, exception); + corInfo.CompileMethod(methodCodeNodeNeedingCode, throwingIL); } } From abe9b35f4ff47e4c8766fd7662424e6a48e918c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Thu, 21 Mar 2024 17:22:52 +0100 Subject: [PATCH 4/7] Fix formatting --- src/coreclr/jit/gentree.cpp | 2 +- src/coreclr/jit/gentree.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0c99123965c887..c05b0fa6d65395 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18566,7 +18566,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b } } } - CORINFO_SIG_INFO sig; + CORINFO_SIG_INFO sig; eeGetMethodSig(method, &sig, exactClass); if (sig.retType == CORINFO_TYPE_VOID) { diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index bac88fb2060cfa..98083896292d2f 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -5644,7 +5644,7 @@ struct GenTreeCall final : public GenTree jitstd::vector* gtInlineCandidateInfoList; HandleHistogramProfileCandidateInfo* gtHandleHistogramProfileCandidateInfo; - ExactContextInfo* gtExactContextInfo; + ExactContextInfo* gtExactContextInfo; CORINFO_GENERIC_HANDLE compileTimeHelperArgumentHandle; // Used to track type handle argument of dynamic helpers void* gtDirectCallAddress; // Used to pass direct call address between lower and codegen }; From fdf20e12bc0a04b04ab12a9cddde4bfa44bf770a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Thu, 21 Mar 2024 17:52:02 +0100 Subject: [PATCH 5/7] Include method name too --- src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 17772b3b52a445..a5d5ba1978c527 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1200,7 +1200,7 @@ private void getMethodSig(CORINFO_METHOD_STRUCT_* ftn, CORINFO_SIG_INFO* sig, CO else { Debug.Assert(type.HasSameTypeDefinition(method.OwningType), - $"{type.GetDisplayName()} has a different definition from {method.OwningType.GetDisplayName()}"); + $"{type.GetDisplayName()} is a different type definition from the parent of {method.GetDisplayName()}"); Instantiation methodInst = method.Instantiation; Debug.Assert(type is InstantiatedType, $"{type.GetDisplayName()} is not a {nameof(InstantiatedType)}"); method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), (InstantiatedType)type); From 6dffb87121a34fd7eb8b7582c0a379de6e0af125 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Sat, 15 Jun 2024 00:45:42 +0200 Subject: [PATCH 6/7] Remove unrelated change --- .../Compiler/RyuJitCompilation.cs | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs index cf6cbd7777d999..d25a14f5765312 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs @@ -193,7 +193,6 @@ private void CompileSingleMethod(CorInfoImpl corInfo, MethodCodeNode methodCodeN try { corInfo.CompileMethod(methodCodeNodeNeedingCode); - return; } catch (TypeSystemException ex) { @@ -201,21 +200,23 @@ private void CompileSingleMethod(CorInfoImpl corInfo, MethodCodeNode methodCodeN } } - if (exception is TypeSystemException.InvalidProgramException - && method.OwningType is MetadataType mdOwningType - && mdOwningType.HasCustomAttribute("System.Runtime.InteropServices", "ClassInterfaceAttribute")) + if (exception != null) { - Logger.LogWarning(method, DiagnosticId.COMInteropNotSupportedInFullAOT); - } - - if ((_compilationOptions & RyuJitCompilationOptions.UseResilience) != 0) - Logger.LogMessage($"Method '{method}' will always throw because: {exception.Message}"); - else - Logger.LogError($"Method will always throw because: {exception.Message}", 1005, method, MessageSubCategory.AotAnalysis); + // Try to compile the method again, but with a throwing method body this time. + MethodIL throwingIL = TypeSystemThrowingILEmitter.EmitIL(method, exception); + corInfo.CompileMethod(methodCodeNodeNeedingCode, throwingIL); - // Try to compile the method again, but with a throwing method body this time. - MethodIL throwingIL = TypeSystemThrowingILEmitter.EmitIL(method, exception); - corInfo.CompileMethod(methodCodeNodeNeedingCode, throwingIL); + if (exception is TypeSystemException.InvalidProgramException + && method.OwningType is MetadataType mdOwningType + && mdOwningType.HasCustomAttribute("System.Runtime.InteropServices", "ClassInterfaceAttribute")) + { + Logger.LogWarning(method, DiagnosticId.COMInteropNotSupportedInFullAOT); + } + if ((_compilationOptions & RyuJitCompilationOptions.UseResilience) != 0) + Logger.LogMessage($"Method '{method}' will always throw because: {exception.Message}"); + else + Logger.LogError($"Method will always throw because: {exception.Message}", 1005, method, MessageSubCategory.AotAnalysis); + } } } From 120fd20b6b8b7ee4d7ce2a5e6a04d42d18c4385b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Sat, 15 Jun 2024 01:04:06 +0200 Subject: [PATCH 7/7] Apply Michals patch --- .../ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs index 46e3b1af0e4628..9699cf35cfa84c 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs @@ -1462,7 +1462,14 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO // the method with a method the intrinsic expands into. If it's not the special intrinsic, // method stays unchanged. var methodIL = (MethodIL)HandleToObject((void*)pResolvedToken.tokenScope); - targetMethod = _compilation.ExpandIntrinsicForCallsite(targetMethod, methodIL.OwningMethod); + MethodDesc callsiteMethod = _compilation.ExpandIntrinsicForCallsite(targetMethod, methodIL.OwningMethod); + if (targetMethod != callsiteMethod) + { + Debug.Assert(!pResult->exactContextNeedsRuntimeLookup); + Debug.Assert(!callsiteMethod.HasInstantiation && !targetMethod.HasInstantiation); + pResult->contextHandle = contextFromType(callsiteMethod.OwningType); + targetMethod = callsiteMethod; + } // For multidim array Address method, we pretend the method requires a hidden instantiation argument // (even though it doesn't need one). We'll actually swap the method out for a differnt one with