Skip to content
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.

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

Wasm implementation #21

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

kant2002
Copy link
Contributor

This is just @yowl work from his repo.

I would like to ask him to create proper PR with just code changes and instruction, then I can improve tooling, so whole thing can be built using dotnet publish workflow.

This PR is just for discussion whole idea, and give additional visibility for WASM work https://github.com/yowl/SeeSharpSnake/tree/wasm4

@yowl
Copy link
Contributor

yowl commented Jan 15, 2022

I did hack it about a bit due to wasm4. The framebuffer is provided, there's no main, sprites not characters, no PAL layer. Think those were the main things. What would be best, put all those things behind #if defs?

@yowl
Copy link
Contributor

yowl commented Jan 16, 2022

I'm probably not going to get to this as I have to hack the compiler to make it work and I don't understand why, so not worth pursuing unless that problem can be resolved, sorry. Basically the static field address calculation is screwed up when compiling or linking for w4 and goes off into random memory.

@kant2002
Copy link
Contributor Author

I'm probably not going to get to this as I have to hack the compiler to make it work and I don't understand why.

I think that's also reasonable. I only trying to consolidate knowledge points if possible. I then probable would go to your fork and try to make dotnet publish work there. Already send cleanup PR.

@MichalStrehovsky
Copy link
Owner

I think we just need to redo the sprites to be static ReadOnlySpan<byte> Xyz => new byte[] { 1,2,3}; to avoid the problem described in dotnet/runtimelab#1822 (comment).

This obviously requires bringing as much of ReadOnlySpan as needed.

@yowl
Copy link
Contributor

yowl commented Jan 16, 2022 via email

@yowl
Copy link
Contributor

yowl commented Jan 16, 2022

Basically

unsafe ref struct Game
{
    static Game _game;

is not allowed . ref structs are on the stack, so they cant be used for static fields I guess. Problem then is how do I structure this given that there is no main? I need some static methods that can be called from W4

@yowl
Copy link
Contributor

yowl commented Jan 16, 2022

Would I be able to create the Game struct in the start call (called once per W4 lifetime), then clobber that over some static byte[] arrary. Then in at the start of update Unsafe.As that back into the ref struct? Too hacky?

@MichalStrehovsky
Copy link
Owner

Why do you need to make the Game a ref struct?

My suggestion above is basically this: https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQ6LAE4FcDGwABAOJhQCm6A3ukfUXEgGxEBKFYAJgPIB2AGwCeAZQAOYPgB4ARkOAUAfEQAaQgF5EAvMr4UA7kTkKA2gF0i1IkgA0RBHYDMRAL4BudC6A===

It doesn't require making Game a ref struct and compiles to two movs (that are also quite inlineable).

@yowl
Copy link
Contributor

yowl commented Jan 17, 2022 via email

@MichalStrehovsky
Copy link
Owner

The newobj is for ReadOnlySpan, which is a valuetype. It won't be a heap allocation even with optimizations disabled.

@yowl
Copy link
Contributor

yowl commented Jan 17, 2022

It won't be a heap allocation

How does that happen, when I look at the IR, I see the helper call for RhpNewArray

------------ BB02 [000..01C) (return), preds={BB01} succs={}
               [000028] ------------                 IL_OFFSET void   IL offset: 0x0
N003 (  1,  4) [000002] H-----------         t2 =    CNS_INT(h) int    0x420040 class
N004 (  1,  1) [000001] ------------         t1 =    CNS_INT   int    2
                                                  /--*  t2     int    arg0 in rcx
                                                  +--*  t1     int    arg1 in rdx
N005 ( 16,  9) [000003] --CXG-------         t3 = *  CALL help ref    HELPER.CORINFO_HELP_NEWARR_1_VC

@MichalStrehovsky
Copy link
Owner

The il in sharplab is:

IL_0000: ldsflda valuetype ''/'__StaticArrayInitTypeSize=3' ''::'039058C6F2C0CB492C533B0A4D14EF77CC0F78ABCCCED5287D84A1A2011CFB81'
IL_0005: ldc.i4.3
IL_0006: newobj instance void valuetype [System.Private.CoreLib]System.ReadOnlySpan`1::.ctor(void*, int32)
IL_000b: ret

The field is RVA static (ie the data is in the rdata section of the executable). If you see newarr, Roslyn doesn't think it's the ReadOnlySpan in your corelib.

@yowl
Copy link
Contributor

yowl commented Jan 17, 2022

Aha! Right, better download Roslyn to see how it decides. Thanks! I guess [System.Private.CoreLib] is my problem

@MichalStrehovsky
Copy link
Owner

In the Roslyn tree there should be ReadOnlySpans they use for testing that the compiler should understand as ReadOnlySpan.

This one might be it:

https://github.com/dotnet/roslyn/blob/19d315955b91f40c374f94a8f2674b8956d912f4/src/Compilers/Test/Utilities/CSharp/TestSources.cs#L85-L156

Once you have something Roslyn recognizes, you might try to make it smaller (e.g. the enumerator might not actually be needed, but who knows).

@yowl
Copy link
Contributor

yowl commented Jan 18, 2022

Thanks, that saved me some debugging. Looks like this ctor is important

        unsafe public ReadOnlySpan(void* pointer, int length)
        {
            _pointer = new ByReference<T>(ref Unsafe.As<byte, T>(ref *(byte*)pointer));
            _length = length;
        }

The enumerator and Span stuff I dropped and looks ok

image

@yowl
Copy link
Contributor

yowl commented Jan 18, 2022

Hmm, I guess that's obvious looking at the IL, oh well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants