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

Add common type conversions #2238

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Doprez
Copy link
Contributor

@Doprez Doprez commented Apr 27, 2024

PR Details

This PR adds some useful extensions that are commonly used with external libraries since Numerics has become the standard.

Related Issue

#2131 this is a great example where something like this could be used.

Motivation and Context

the Bepu branch has this built into the library but its also used in the model importer as well as any library that may need these fast conversions.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@Doprez Doprez marked this pull request as ready for review April 29, 2024 21:34
@Doprez Doprez changed the title [WIP] Add common type conversions Add common type conversions May 2, 2024
@timcassell
Copy link
Contributor

timcassell commented May 13, 2024

I think if you pass by ref (use in and Unsafe.AsRef) and apply [MethodImpl(MethodImplOptions.AggressiveInlining)], the conversions should be basically free.

@Doprez
Copy link
Contributor Author

Doprez commented May 14, 2024

I did some quick tests with Inlining and the performance was the same or similar on a few runs. It seems like the compiler does not make any significant changes with or witthout it to the compiled code, I tested with the Bepu library mentioned above.

Unless Im missing something I cant seem to use Unsafe.AsRef for conversion to another type?

@Eideren
Copy link
Collaborator

Eideren commented May 14, 2024

@timcassell
Copy link
Contributor

timcassell commented May 14, 2024

Ah nice, BitCast looks awesome. I was imagining it like this, which it looks like BitCast creates the same code in a cleaner way.

public static Vector2 ToStride(this in NVector2 v) => Unsafe.As<NVector2, Vector2>(ref Unsafe.AsRef(v));

The in modifier is to prevent pass-by-value copies (which you can see in the asm of TestBase vs TestRef).

as the input doesn't need to be passed as ref/in to have improved assembly (see asm for C.TestBitcast).

The assembly for Bitcast vs BitcastIn is different though. It looks like it's simply better with in (in case the JIT doesn't actually inline it, which is not guaranteed).

C.Bitcast(System.Numerics.Vector2)
    L0000: vzeroupper
    L0003: vmovq xmm0, rcx
    L0008: vmovq rax, xmm0
    L000d: ret

C.BitcastIn(System.Numerics.Vector2 ByRef)
    L0000: mov rax, [rcx]
    L0003: ret

@Eideren Eideren marked this pull request as draft June 7, 2024 20:03
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