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

New codegen for inline array fields #140

Merged
merged 9 commits into from
Feb 26, 2021

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 23, 2021

Closes #117

TODO:

  • Add range checks to the fallback indexer implementation to parallel span range checks
  • Ask JaredPar to review the generated code and see if ref p[index] should be ref Unsafe.AsRef<uint>(p + index)

Currently generated code (odd whitespace is not my fault; tracked by #23):

[StructLayout(LayoutKind.Sequential, Pack = 1)]
internal struct __dwReserved_4
{
    internal uint _1, _2, _3, _4;
    internal ref uint this[int index]
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        get
        {
            unsafe
            {
                fixed (uint *p = &_1)
                    return ref p[index];
            }
        }
    }
}

@AArnott I like keeping unsafe scoped as tightly as possible, meaning I use the statement form if the signature itself isn't unsafe, but let me know if this is too weird or something, or if using an embedded statement instead of a block inside the fixed statement is a problem.

@jnm2 jnm2 marked this pull request as ready for review February 23, 2021 14:11
@jnm2 jnm2 force-pushed the new_inline_array_codegen branch 5 times, most recently from cf685fc to 3ea4338 Compare February 23, 2021 19:02
internal uint dwWidth;
internal uint dwHeight;
internal __dwReserved_4 dwReserved;
[StructLayout(LayoutKind.Sequential, Pack = 1)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tannergooding were you saying we don't need to specify the pack size?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Inline arrays are just like regular arrays and the natural padding of a struct is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(assuming the metadata doesn't specify otherwise)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw that too. I can add this change to this PR, no problem. 👍

@AArnott
Copy link
Member

AArnott commented Feb 24, 2021

I've asked JaredPar on another thread to review and will report back here.

@AArnott AArnott self-assigned this Feb 25, 2021
@AArnott
Copy link
Member

AArnott commented Feb 26, 2021

Just an update: I am actively working on this and have lots of changes coming. I'm just working to get all tests passing. The new extension methods we have to declare bring on some new challenges.

@AArnott AArnott merged commit 8f6e829 into microsoft:main Feb 26, 2021
@jnm2
Copy link
Contributor Author

jnm2 commented Feb 26, 2021

@AArnott Thanks for completing this!

@AArnott
Copy link
Member

AArnott commented Feb 26, 2021

Thanks for starting it, @jnm2! Particularly because it led us to discover bugs in how we did it before.

AArnott added a commit that referenced this pull request May 6, 2022
Update GitHub Actions to more closely resemble AzP
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.

Avoid using Span<T> on TFMs older than net45
3 participants