CppCodeGen: implement shared generics and support generic virtual methods #6467

kbaladurin · 2018-10-17T15:31:50Z

This patch fixes #6147

kbaladurin · 2018-10-17T15:33:09Z

kbaladurin · 2018-10-17T15:33:23Z

jkotas · 2018-10-17T18:45:13Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/BlockReflectionTypeMapNode.cs

@@ -13,7 +13,7 @@ namespace ILCompiler.DependencyAnalysis
    /// <summary>
    /// Represents a hashtable of all type blocked from reflection.
    /// </summary>
-    internal sealed class BlockReflectionTypeMapNode : ObjectNode, ISymbolDefinitionNode
+    public sealed class BlockReflectionTypeMapNode : ObjectNode, ISymbolDefinitionNode


Why do all these things types need to be public now?

I think this is forced by CppCodegen living in a separate assembly and having to special cases these node types in the CppWriter. I never really liked that that's where we ended up with for CppWriter (the other object writers only deal with ObjectNode and don't have to be aware of the specific type).

It might be nice at some point to see if we can make CppWriter more general purpose so that it doesn't have to be aware of specific node types. But that's a future nice to have.

jkotas · 2018-10-17T18:46:16Z

src/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/ExternalReferencesTable.cs

@@ -161,8 +151,13 @@ unsafe public IntPtr GetFieldAddressFromIndex(uint index)
                throw new BadImageFormatException();

            // TODO: indirection through IAT
-            int* pRelPtr32 = &((int*)_elements)[index];


It may be nice to submit the supporting self-contained fixes like this one as separate PRs.`

Do you think it's better to submit changes in ExternalReferencesTable and ExternalReferencesTableNode as separate PR?

Up to you ... splitting large change into multiple smaller PRs that make sense on its own makes it easier to push the changes through.

MichalStrehovsky

I'll look at the changes in CppWriter and importer later, but this looks awesome.

Would it be possible to start running the tests/src/Simple/Generics test now (by deleting the no_cpp file in the directory)? (Even if we have to put some parts under #if !CODEGEN_CPP).

MichalStrehovsky · 2018-10-17T16:41:59Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/TypeMetadataNode.cs

@@ -26,6 +26,7 @@ internal class TypeMetadataNode : DependencyNodeCore<NodeFactory>

        public TypeMetadataNode(MetadataType type)
        {
+            EcmaType t = (EcmaType)type;


This looks unnecessary

yes, thank you!

MichalStrehovsky · 2018-10-17T19:59:03Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/BlockReflectionTypeMapNode.cs

@@ -13,7 +13,7 @@ namespace ILCompiler.DependencyAnalysis
    /// <summary>
    /// Represents a hashtable of all type blocked from reflection.
    /// </summary>
-    internal sealed class BlockReflectionTypeMapNode : ObjectNode, ISymbolDefinitionNode
+    public sealed class BlockReflectionTypeMapNode : ObjectNode, ISymbolDefinitionNode


I think this is forced by CppCodegen living in a separate assembly and having to special cases these node types in the CppWriter. I never really liked that that's where we ended up with for CppWriter (the other object writers only deal with ObjectNode and don't have to be aware of the specific type).

It might be nice at some point to see if we can make CppWriter more general purpose so that it doesn't have to be aware of specific node types. But that's a future nice to have.

morganbr · 2018-10-17T23:43:21Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ExternalReferencesTableNode.cs

@@ -102,6 +102,10 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
                    // TODO: set low bit if the linkage of the symbol is IAT_PVALUE.
                    builder.EmitReloc(symbolAndDelta.Symbol, RelocType.IMAGE_REL_BASED_RELPTR32, symbolAndDelta.Delta);
                }
+                else if (factory.Target.Abi == TargetAbi.CppCodegen)


@MichalStrehovsky , should this be based off of TargetDetails rather than Cpp?

Yes, good point. We should shuffle this a little and put the TargetAbi.ProjectN case first (that one will stay weird as long as we have the Binder), and then just use factory.Target.SupportsRelativePointers to decide whether the reloc is a full pointer, or just a relative address. The pointer reloc case is also relevant for webassembly.

Done. Thank you for suggestion!

MichalStrehovsky · 2018-10-18T01:30:50Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs

    {
        public CanonicalEETypeNode(NodeFactory factory, TypeDesc type) : base(factory, type)
        {
            Debug.Assert(!type.IsCanonicalDefinitionType(CanonicalFormKind.Any));
            Debug.Assert(type.IsCanonicalSubtype(CanonicalFormKind.Any));
            Debug.Assert(type == type.ConvertToCanonForm(CanonicalFormKind.Specific));
-            Debug.Assert(!type.IsMdArray);


We should not be ending up with canonical EETypes for MdArrays - the template type loader doesn't need templates for these because there's nothing generic about them (they don't implement the generic interfaces that normal arrays do, such as IEnumerable<T>, etc.).

What stack are you hitting this assert from?

I'm hitting following stacktrace:

Compiling [Hello]Hello.Program+B.M<__Canon>(__Canon) AddTypeReference owningType: [S.P.CoreLib]System.__Canon[,] Assertion Failed at ILCompiler.DependencyAnalysis.CanonicalEETypeNode..ctor(NodeFactory factory, TypeDesc type) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs:line 29 at ILCompiler.DependencyAnalysis.NodeFactory.<CreateNodeCaches>b__36_1(TypeDesc type) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs:line 194 at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory) at ILCompiler.DependencyAnalysis.NodeFactory.NodeCache`2.GetOrAdd(TKey key) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs:line 150 at ILCompiler.DependencyAnalysis.NodeFactory.ConstructedTypeSymbol(TypeDesc type) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs:line 527 at Internal.IL.ILImporter.AddTypeDependency(TypeDesc type, Boolean constructed) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 3233 at Internal.IL.ILImporter.AddTypeReference(TypeDesc type, Boolean constructed) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 3211 at Internal.IL.ILImporter.ImportNewObjArray(TypeDesc owningType, MethodDesc method) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 1174 at Internal.IL.ILImporter.ImportCall(ILOpcode opcode, Int32 token) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 1290 at Internal.IL.ILImporter.ImportBasicBlock(BasicBlock basicBlock) in /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/TypeSystem/IL/ILImporter.cs:line 616 at Internal.IL.ILImporter.ImportBasicBlocks() in /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/TypeSystem/IL/ILImporter.cs:line 327 at Internal.IL.ILImporter.Compile(CppMethodCodeNode methodCodeNodeNeedingCode) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 758 at ILCompiler.CppCodeGen.CppWriter.CompileMethod(CppMethodCodeNode methodCodeNodeNeedingCode) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/CppWriter.cs:line 553 at ILCompiler.CppCodegenCompilation.ComputeDependencyNodeDependencies(List`1 obj) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/Compiler/CppCodegenCompilation.cs:line 71 at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeDependencies(List`1 deferredStaticDependencies) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.DependencyAnalysisFramework/src/DependencyAnalyzer.cs:line 139 at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes() in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.DependencyAnalysisFramework/src/DependencyAnalyzer.cs:line 262 at ILCompiler.CppCodegenCompilation.CompileInternal(String outputFile, ObjectDumper dumper) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/Compiler/CppCodegenCompilation.cs:line 47 at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String outputFile, ObjectDumper dumper) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/Compilation.cs:line 333 at ILCompiler.Program.Run(String[] args) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler/src/Program.cs:line 514 at ILCompiler.Program.Main(String[] args) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler/src/Program.cs:line 670

when compiling this method:

public override void M<T>(T t) { T[,] a = new T[2, 2]; System.Console.WriteLine("Virtual B " + t + " " + a); }

Let's change the assert to !type.IsMdArray || factory.Target.Abi == TargetAbi.CppCodegen. Generating these for MdArrays still results in useless data structures, but due to how CppCodegen needs to operate, we'll have to live with it there.

Done. Thank you!

MichalStrehovsky · 2018-10-18T01:39:37Z

src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs

+            }
+            else
+            {
+                Debug.Assert(_method.RequiresInstArg() || _method.AcquiresInstMethodTableFromThis());


RequiresInstArg [](start = 37, length = 15)

RequiresInstMethodTableArg matches more closely what we need to assert than RequiresInstArg

kbaladurin · 2018-10-25T13:20:56Z

I've checked Generics tests. All tests are passed except following:

TestReflectionInvoke (fails due to missing reflection support)
TestFieldAccess (fails due to missing reflection support)
TestMDArrayAddressMethod (fails due to missing exception support)
TestNativeLayoutGeneration (fails due to missing null check before call)

MichalStrehovsky

Looks good to me otherwise.

MichalStrehovsky · 2018-10-25T15:42:31Z

src/ILCompiler.CppCodeGen/src/CppCodeGen/EvaluationStack.cs

+    {
+        public bool IsVirtual { get; }
+
+        public LdFtnTokenEntry(StackValueKind kind, string name, MethodDesc token, bool isVirtual, TypeDesc type = null) : base(kind, name, token, type)


Should we override the Duplicate method?

Yes, thank you for the note!

MichalStrehovsky · 2018-10-25T15:44:05Z

src/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/ExternalReferencesTable.cs

-            // TODO: indirection through IAT
-            int* pRelPtr32 = &((int*)_elements)[index];
-            return (IntPtr)((byte*)pRelPtr32 + *pRelPtr32);
+            return GetFieldAddressFromIndex(index);


Maybe we should rename to GetAddressFromIndex since this is now more general-purpose.

MichalStrehovsky · 2018-10-25T15:47:41Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs

    {
        public CanonicalEETypeNode(NodeFactory factory, TypeDesc type) : base(factory, type)
        {
            Debug.Assert(!type.IsCanonicalDefinitionType(CanonicalFormKind.Any));
            Debug.Assert(type.IsCanonicalSubtype(CanonicalFormKind.Any));
            Debug.Assert(type == type.ConvertToCanonForm(CanonicalFormKind.Specific));
-            Debug.Assert(!type.IsMdArray);


Let's change the assert to !type.IsMdArray || factory.Target.Abi == TargetAbi.CppCodegen. Generating these for MdArrays still results in useless data structures, but due to how CppCodegen needs to operate, we'll have to live with it there.

MichalStrehovsky · 2018-10-25T16:28:52Z

src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs

+
+            Append("if (");
+            Append(fatPtr);
+            Append(" & 0x2) {");


Can you change this and below to use FatFunctionPointerConstants.Offset instead? You might need to add RuntimeConstants.cs to the project, but that should be fine.

(Also below.)

MichalStrehovsky · 2018-10-25T16:47:33Z

src/ILCompiler.CppCodeGen/src/CppCodeGen/CppWriter.cs

+            Out.Write(sb.ToString());
+        }
+
+        public TypeDesc ConvertToCanonFormIfNecessary(TypeDesc type, CanonicalFormKind policy)


I think this can just be:

public static TypeDesc CanonicallyNormalized(this TypeDesc type) { if (type.IsCanonicalSubtype(CanonicalFormKind.Any)) return type.ConvertToCanonForm(CanonicalFormKind.Specific); return type; }

The pointers and byrefs shouldn't require special casing, and we don't need to specify canonical form as a parameter (if the type happens to have CanonicalFormKind.Universal types in it, the ConvertToCanonForm will to the necessary upgrade).

This method could also be static/extension method, and we could place it in TypeExtensions.cs. I know at least one more place that could use it.

I used special casing for pointers and byrefs to do conversions like this: Dictionary<String, Canon>& -> Dictionary<Canon, Canon>&. ConvertToCanonForm converts such types to Canon& that isn't convenient for cpp codegen.

I see. CppCodegen requires different canonical forms than what I'm used to. This looks good.

MichalStrehovsky · 2018-10-25T16:52:18Z

I've checked Generics tests. All tests are passed except following

That's great to hear! Could you please put the things that don't work under #if !CODEGEN_CPP (same as we do in the PInvoke test) and delete the no_cpp file in the test directory so that we run it regularly in the CI?

Could you also move the definition of CODEGEN_CPP to SimpleTest.targets while you're at it so that we don't duplicate it?

kbaladurin · 2018-10-26T09:40:55Z

Thank you for the review! I've added patch that enables generics test for CppCodeGen.

…hods

MichalStrehovsky

Thank you!

jkotas reviewed Oct 17, 2018

View reviewed changes

MichalStrehovsky reviewed Oct 17, 2018

View reviewed changes

morganbr reviewed Oct 17, 2018

View reviewed changes

MichalStrehovsky reviewed Oct 18, 2018

View reviewed changes

kbaladurin force-pushed the master branch 5 times, most recently from 2e149a4 to 24442be Compare October 18, 2018 15:54

MichalStrehovsky mentioned this pull request Oct 21, 2018

wasm: typeof(T) fails at runtime #5842

Closed

kbaladurin force-pushed the master branch from 24442be to 30a641b Compare October 25, 2018 13:09

MichalStrehovsky reviewed Oct 25, 2018

View reviewed changes

kbaladurin force-pushed the master branch from 30a641b to f0a974a Compare October 26, 2018 08:42

kbaladurin changed the title ~~[WIP] CppCodeGen: implement shared generics and support generic virtual methods~~ CppCodeGen: implement shared generics and support generic virtual methods Oct 26, 2018

Konstantin Baladurin added 2 commits October 26, 2018 14:23

CppCodeGen: implement shared generics and support generic virtual met…

b7d48b0

…hods

Partially enable Generics test for CppCodeGen

7f135eb

kbaladurin force-pushed the master branch from f0a974a to 7f135eb Compare October 26, 2018 11:23

MichalStrehovsky approved these changes Oct 29, 2018

View reviewed changes

MichalStrehovsky merged commit 698a30e into dotnet:master Oct 29, 2018

yowl mentioned this pull request Oct 31, 2018

Add null checks for field load/stores #6399

Closed

MichalStrehovsky mentioned this pull request Apr 3, 2019

wasm: support Generic Virtual Method Calls & Shared Generics (was: some linq methods fail to compile) #7248

Closed

MichalStrehovsky mentioned this pull request Jul 20, 2021

DrawImageProcessor AOT Issue SixLabors/ImageSharp#1703

Closed

4 tasks

drcjt mentioned this pull request Oct 4, 2024

No sharing of generic instantiations using reference types drcjt/CSharp-80#531

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CppCodeGen: implement shared generics and support generic virtual methods #6467

CppCodeGen: implement shared generics and support generic virtual methods #6467

kbaladurin commented Oct 17, 2018

kbaladurin commented Oct 17, 2018

kbaladurin commented Oct 17, 2018

jkotas Oct 17, 2018

MichalStrehovsky Oct 17, 2018

jkotas Oct 17, 2018

kbaladurin Oct 18, 2018

jkotas Oct 18, 2018

MichalStrehovsky left a comment

MichalStrehovsky Oct 17, 2018

kbaladurin Oct 18, 2018

MichalStrehovsky Oct 17, 2018

morganbr Oct 17, 2018

MichalStrehovsky Oct 18, 2018

kbaladurin Oct 18, 2018

MichalStrehovsky Oct 18, 2018

kbaladurin Oct 18, 2018

MichalStrehovsky Oct 25, 2018

kbaladurin Oct 26, 2018

MichalStrehovsky Oct 18, 2018

kbaladurin Oct 18, 2018

kbaladurin commented Oct 25, 2018

MichalStrehovsky left a comment

MichalStrehovsky Oct 25, 2018

kbaladurin Oct 26, 2018

MichalStrehovsky Oct 25, 2018

MichalStrehovsky Oct 25, 2018

MichalStrehovsky Oct 25, 2018

MichalStrehovsky Oct 25, 2018

kbaladurin Oct 26, 2018

MichalStrehovsky Oct 26, 2018

MichalStrehovsky commented Oct 25, 2018

kbaladurin commented Oct 26, 2018

MichalStrehovsky left a comment

CppCodeGen: implement shared generics and support generic virtual methods #6467

CppCodeGen: implement shared generics and support generic virtual methods #6467

Conversation

kbaladurin commented Oct 17, 2018

kbaladurin commented Oct 17, 2018

kbaladurin commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbaladurin commented Oct 25, 2018

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Oct 25, 2018

kbaladurin commented Oct 26, 2018

MichalStrehovsky left a comment

Choose a reason for hiding this comment