Wasm: Align classes and arrays with 64 bit types at 8 byte #8255
Conversation
| #define GC_ALLOC_FINALIZE 0x1 // TODO: Defined in gc.h | ||
| #define GC_ALLOC_FINALIZE 0x1 // TODO: Defined in gc.h | ||
| #define GC_ALLOC_ALIGN8_BIAS 0x4 // TODO: Defined in gc.h | ||
| #define GC_ALLOC_ALIGN8 0x8 // TODO: Defined in gc.h |
There was a problem hiding this comment.
I tried including gcinterface.h but that seems to have other dependencies
src/Native/Runtime/portable.cpp
Outdated
| @@ -180,36 +181,161 @@ COOP_PINVOKE_HELPER(String *, RhNewString, (EEType * pArrayEEType, int numElemen | |||
|
|
|||
| #endif | |||
| #if defined(USE_PORTABLE_HELPERS) | |||
| struct RawEEType // TODO: defined in common.h | |||
There was a problem hiding this comment.
This is a copy from Bootstrap/common.h. Could have used EEType also...
src/Native/Runtime/portable.cpp
Outdated
| // dummy object for aligning next allocation to 8 that supports Methodtable GetBaseSize (12),HasComponentSize (false) | ||
| static struct RawEEType dummy12ByteEEType = { 0, 0, 12, NULL, 0, 0, 0 }; | ||
|
|
||
| #if defined(HOST_ARM) || defined(HOST_WASM) | ||
| COOP_PINVOKE_HELPER(Object *, RhpNewFinalizableAlign8, (EEType* pEEType)) | ||
| { | ||
| Object * pObject = nullptr; | ||
| /* TODO */ ASSERT_UNCONDITIONALLY("NYI"); |
There was a problem hiding this comment.
Haven't looked at this one, but if its trivial I could fill it out.
There was a problem hiding this comment.
Yes, this can be pretty simple - similar to RhpNewFastMisalign
There was a problem hiding this comment.
Right, I've done it, not committed though as I'm having a hard time getting a test. The only types that I can make have alignment.AsInt == 8 at
I thought a class with a long would be enough
class FinalizableClass
{
long l; // requires 8 alignment
~FinalizableClass()
{
l = 1;
if(l == 1) finalizeCalled = true;
}
}
There was a problem hiding this comment.
Good point. You are right - this is de-facto unreachable. It may be worth it to add a comment about it.
| #ifdef FEATURE_CONSERVATIVE_GC | ||
| && (GCConfig::GetConservativeGC() || interior <= heap_segment_allocated(seg)) | ||
| if ( interior >= heap_segment_allocated(seg)) |
There was a problem hiding this comment.
This is the same as merged in dotnet/runtime#39905 . If this complicates merging, I can leave this PR as WIP.
src/Native/Runtime/portable.cpp
Outdated
|
|
||
| #ifdef HOST_ARM | ||
| // dummy object for aligning next allocation to 8 that supports Methodtable GetBaseSize (12),HasComponentSize (false) |
There was a problem hiding this comment.
This should use the existing g_pFreeObjectEEType
There was a problem hiding this comment.
Thanks, removed in favour of g_pFreeObjectEEType
| @@ -286,8 +287,7 @@ public LayoutInt GetObjectAlignment(LayoutInt fieldAlignment) | |||
| case TargetArchitecture.ARM64: | |||
| return new LayoutInt(8); | |||
| case TargetArchitecture.X86: | |||
| case TargetArchitecture.Wasm32: | |||
| return new LayoutInt(4); | |||
| return new LayoutInt(Math.Max(4, fieldAlignment.AsInt)); | |||
There was a problem hiding this comment.
Think this is old and is causing the CI test failures. I'll try reverting.
| @@ -663,7 +663,6 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, | |||
| while (minAlign.AsInt < cumulativeInstanceFieldPos.AsInt) | |||
| minAlign = new LayoutInt(minAlign.AsInt * 2); | |||
| } | |||
src/Native/Runtime/portable.cpp
Outdated
| @@ -180,36 +181,150 @@ COOP_PINVOKE_HELPER(String *, RhNewString, (EEType * pArrayEEType, int numElemen | |||
|
|
|||
| #endif | |||
| #if defined(USE_PORTABLE_HELPERS) | |||
| #if defined(HOST_ARM) || defined(HOST_WASM) | |||
There was a problem hiding this comment.
Use FEATURE_64BIT_ALIGNMENT here?
src/Native/Runtime/portable.cpp
Outdated
| Object * pObject = nullptr; | ||
| /* TODO */ ASSERT_UNCONDITIONALLY("NYI"); | ||
| size_t size = pEEType->get_BaseSize(); | ||
| size = (size + 7) & ~7; |
There was a problem hiding this comment.
This should not be needed. I do not see it in the asm version either.
Closes #8250
This PR enables the
FEATURE_64BIT_ALIGNMENTfeature for Wasm and implements some of theRhpNew...functions to align arrays and classes at 8 bytes. This will move Wasm closer to enabling threads (although may have to wait for Wasm Exceptions for this as the current emscripten exception handling is not thread safe - this Wasm feature is currently behind a flag in v8).May also improve ARM32 support, but not tried that.