enabled thread static fields in WebAssembly by hippiehunter · Pull Request #4999 · dotnet/corert

hippiehunter · 2017-11-23T03:34:04Z

@morganbr
I've enabled thread static fields, it's generating quite a bit more code as a result and it ended up pulling in some weirdness around virtual calls to System.Type. The EEType wasnt getting output, so I've put in a dependency for the owning type when calling a virtual method. If you have any ideas for a different place to trigger the reference for the EEType that works too.

MichalStrehovsky · 2017-11-23T10:19:16Z

The EEType wasnt getting output, so I've put in a dependency for the owning type when calling a virtual method

That sounds like a bug somewhere.

To put it into a sample: the code below is not supposed to have an EEType for MyProgram generated and for RyuJIT codegen, it won't. For CppCodegen it will, but only because CppCodegen predates dependency analysis infrastructure and it was never fully hooked up into it. Wasm codegen should be more like what we do with RyuJIT than what we do in CppCodegen.

internal class MyProgram
{
    protected virtual void DoNothing() { }

    private static void Main(string[] args)
    {
        MyProgram p = null;
        if (args == null)
            p.DoNothing();
    }
}

hippiehunter · 2017-11-24T04:03:04Z

How can I call a method via virtual slot without an eetype? Is this case supposed to be devirtualized?

MichalStrehovsky · 2017-11-24T08:23:57Z

How can I call a method via virtual slot without an eetype? Is this case supposed to be devirtualized?

You don't need to directly reference the EEType at the callsite. The EEType is implicitly looked up by dereferencing the this pointer at runtime. The compiler only needs to emit the offset into the EEType to find the slot. The fact that there's no EEType into which the slot number could index doesn't matter; the code is not going to execute (and if it does execute, it will throw a NullReferenceException - because no EEType means we clearly never allocated such type).

The EEType might be required because of how virtual calls got hooked up for WASM, but this can be implemented without the EEType. E.g. a scheme that would likely work is:

Take InterfaceDispatchCellNode.cs as inspiration, with a couple differences:
- The constructor only takes one MethodDesc (the slot defining method)
- In GetData, only do a single Int32 write - the offset into the EEType to find the slot for the slot defining method
- The rest of methods in InterfaceDispatchCellNode.cs can be pretty much taken verbatim (with a couple obvious tweaks
Hook up the new node into WebAssemblyCodegenNodeFactory (take inspiration from how the interface dispatch cell is hooked up in NodeFactory.cs)

Once you have that, to generate a virtual call, you would:

dereference this
dereference the "virtual method slot offset node" that corresponds to the slot defining method that is to be called
add those two numbers
dereference the result and call it

MichalStrehovsky · 2017-11-28T18:33:25Z

-            {
-                slot = LLVM.AddGlobal(Module, LLVM.Int32Type(), globalRefName);
-            }
+            var vtableSlotSymbol = ((WebAssemblyCodegenNodeFactory)_compilation.NodeFactory).VTableSlot(method);


It might be a general improvement to add public new WebAssemblyCodegenNodeFactory NodeFactory property to WebAssemblyCodegenCompilation so that this cast is not necessary. WebAssemblyCodegenCompilationBuilder can just pass it as a strongly typed argument to the constructor and Compilation can save it.

MichalStrehovsky · 2017-11-28T18:34:55Z

+            if (!relocsOnly)
+            {
+                var tableOffset = EETypeNode.GetVTableOffset(factory.Target.PointerSize) / factory.Target.PointerSize;
+                objData.EmitInt(tableOffset + VirtualMethodSlotHelper.GetVirtualMethodSlot(factory, _targetMethod));


Is this multiplied by pointer size at runtime? It might be better to just save it premultiplied by pointer size so that this is a simple addition at runtime.

storing it as a multiple of pointers allows the LLVM/WASM instructions to be shorter because we treat the EEType as a blob of function pointers and this lets us directly index without having to cast anything.

MichalStrehovsky · 2017-11-29T11:40:58Z

 {
    public sealed class WebAssemblyCodegenNodeFactory : NodeFactory
    {
+        private NodeCache<MethodKey, WebAssemblyVTableSlotNode> _vTableSlotNodes;


This can just use MethodDesc as the key. MethodKey is used when the method can be an unboxing stub. It can't in this case.

MichalStrehovsky · 2017-11-29T11:42:54Z

+
+        public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
+        {
+


Nit: unnecessary blank line

MichalStrehovsky · 2017-11-29T11:43:56Z

+
+            if (!relocsOnly)
+            {
+                var tableOffset = EETypeNode.GetVTableOffset(factory.Target.PointerSize) / factory.Target.PointerSize;


While I don't think we would ever want vtable slots to be unaligned, maybe assert that the offset is divisible by pointer size?

MichalStrehovsky · 2017-11-29T11:44:32Z

        internal WebAssemblyCodegenConfigProvider Options { get; }
        internal LLVMModuleRef Module { get; }
-
+        public new WebAssemblyCodegenNodeFactory NodeFactory { get; private set; }


Nit: remove the private set; part to make it a read only property.

morganbr · 2017-11-29T23:06:12Z

                    LLVM.SetInitializer(llvmValue, GetConstZeroArray(field.FieldType.GetElementSize().AsInt));
+                    if (field.IsThreadStatic)
+                    {
+                        LLVM.SetThreadLocal(llvmValue, LLVMMisc.True);


It's cool that LLVM can do this for us 😄. What happens with initialization for this? Can you please also add a test using a thread local? I know we can't test it multithreaded, but it would be good to just ensure reading/writing works right.

@hippiehunter what I meant about initialization is that .NET thread static fields are supposed to be zero/null initialized. Does LLVM do that for us or do you also need to do some sort of initialization?

I've added a test to validate the initial value of zero and that works. My understanding is that this should all be managed by the call to SetInitializer directly above.

morganbr

Looks good, thanks, @hippiehunter!

enabled thread static fields

307b13b

changed vtable dispatch to not be dependent on EETypeNode

64088d0

MichalStrehovsky reviewed Nov 28, 2017

View reviewed changes

added typed node factory

de70975

MichalStrehovsky approved these changes Nov 29, 2017

View reviewed changes

morganbr reviewed Nov 29, 2017

View reviewed changes

hippiehunter added 2 commits November 29, 2017 19:13

removed whitespace, added test, removed unneeded things

a23c37b

added initial value test for thread static field

33b0755

morganbr approved these changes Nov 30, 2017

View reviewed changes

morganbr merged commit d80ed08 into dotnet:master Nov 30, 2017

MichalStrehovsky mentioned this pull request Aug 7, 2018

Fix WebAssembly sealed calls #6180

Merged

yowl mentioned this pull request Dec 28, 2018

wasm: Main thread Sleep fails due to missing initialisation? #6733

Closed

MichalStrehovsky mentioned this pull request Jul 9, 2019

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

Closed


		public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
		{

Uh oh!

Conversation

hippiehunter commented Nov 23, 2017

Uh oh!

MichalStrehovsky commented Nov 23, 2017

Uh oh!

hippiehunter commented Nov 24, 2017

Uh oh!

MichalStrehovsky commented Nov 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganbr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants