Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Wasm: Align classes and arrays with 64 bit types at 8 byte #8255

Merged
merged 7 commits into from
Aug 13, 2020
Merged

Wasm: Align classes and arrays with 64 bit types at 8 byte #8255

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9595277
enabled 8 byte alignment for arrays and classes that require it.
yowl Aug 5, 2020
62f5831
remove enum, use constant
yowl Aug 5, 2020
6e6aa32
Remove copy of RawEEType and use g_pFreeObjectEEType
yowl Aug 5, 2020
9a31fdd
revert some old changes that are wrong
yowl Aug 5, 2020
6a7fd40
really revert this time.
yowl Aug 5, 2020
38aae2a
feedback - remove size alignment, add comment for finalizable align8
yowl Aug 13, 2020
b294e94
Change assert message from NYI to UNREACHABLE
yowl Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Common/src/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,10 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
while (minAlign.AsInt < cumulativeInstanceFieldPos.AsInt)
minAlign = new LayoutInt(minAlign.AsInt * 2);
}
Copy link
Member

@jkotas jkotas Aug 5, 2020

Choose a reason for hiding this comment

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

Nit: Unnecessary WS change


if (requiresAlign8 && minAlign.AsInt < 8)
{
minAlign = new LayoutInt(8);
}
SizeAndAlignment instanceByteSizeAndAlignment;
var instanceSizeAndAlignment = ComputeInstanceSize(type, cumulativeInstanceFieldPos, minAlign, 0/* specified field size unused */, out instanceByteSizeAndAlignment);

Expand Down
6 changes: 3 additions & 3 deletions src/Common/src/TypeSystem/Common/TargetDetails.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,8 @@ public LayoutInt GetObjectAlignment(LayoutInt fieldAlignment)
switch (Architecture)
{
case TargetArchitecture.ARM:
// ARM supports two alignments for objects on the GC heap (4 byte and 8 byte)
case TargetArchitecture.Wasm32:
// ARM & Wasm32 support two alignments for objects on the GC heap (4 byte and 8 byte)
if (fieldAlignment.IsIndeterminate)
return LayoutInt.Indeterminate;

Expand All @@ -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));
Copy link
Contributor Author

@yowl yowl Aug 5, 2020

Choose a reason for hiding this comment

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

Think this is old and is causing the CI test failures. I'll try reverting.

default:
throw new NotSupportedException();
}
Expand Down
4 changes: 2 additions & 2 deletions src/ILCompiler.Compiler/src/Compiler/TypeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,11 @@ public static bool IsArrayTypeWithoutGenericInterfaces(this TypeDesc type)

/// <summary>
/// Determines whether an object of type '<paramref name="type"/>' requires 8-byte alignment on
/// 32bit ARM architectures.
/// 32bit ARM or 32bit Wasm architectures.
/// </summary>
public static bool RequiresAlign8(this TypeDesc type)
{
if (type.Context.Target.Architecture != TargetArchitecture.ARM)
if (type.Context.Target.Architecture != TargetArchitecture.ARM && type.Context.Target.Architecture != TargetArchitecture.Wasm32)
{
return false;
}
Expand Down
13 changes: 11 additions & 2 deletions src/ILCompiler.WebAssembly/src/CodeGen/ILToWebAssemblyImporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4944,9 +4944,18 @@ private void ImportNewArray(int token)
else
{
arguments = new StackEntry[] { new LoadExpressionEntry(StackValueKind.ValueType, "eeType", GetEETypePointerForTypeDesc(runtimeDeterminedArrayType, true), eeTypeDesc), sizeOfArray };
//TODO: call GetNewArrayHelperForType from JitHelper.cs (needs refactoring)
}
PushNonNull(CallRuntime(_compilation.TypeSystemContext, InternalCalls, "RhpNewArray", arguments, runtimeDeterminedArrayType));
var helper = GetNewArrayHelperForType(runtimeDeterminedArrayType);
PushNonNull(CallRuntime(_compilation.TypeSystemContext, InternalCalls, helper, arguments, runtimeDeterminedArrayType));
}

//TODO: copy of the same method in JitHelper.cs but that is internal
public static string GetNewArrayHelperForType(TypeDesc type)
{
if (type.RequiresAlign8())
return "RhpNewArrayAlign8";

return "RhpNewArray";
}

LLVMValueRef GetGenericContext()
Expand Down
1 change: 1 addition & 0 deletions src/Native/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ elseif(CLR_CMAKE_TARGET_ARCH STREQUAL wasm)
set(CLR_CMAKE_PLATFORM_ARCH_WASM 1)
add_definitions(-DTARGET_WASM=1)
add_definitions(-DHOST_WASM=1)
add_definitions(-DFEATURE_64BIT_ALIGNMENT=1)
else()
clr_unknown_arch()
endif()
Expand Down
139 changes: 127 additions & 12 deletions src/Native/Runtime/portable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
#include "GCMemoryHelpers.inl"

#if defined(USE_PORTABLE_HELPERS)

EXTERN_C REDHAWK_API void* REDHAWK_CALLCONV RhpGcAlloc(EEType *pEEType, UInt32 uFlags, UIntNative cbSize, void * pTransitionFrame);
EXTERN_C REDHAWK_API void* REDHAWK_CALLCONV RhpPublishObject(void* pObject, UIntNative cbSize);

Expand Down Expand Up @@ -88,7 +87,9 @@ COOP_PINVOKE_HELPER(Object *, RhpNewFast, (EEType* pEEType))
return pObject;
}

#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
Copy link
Contributor Author

@yowl yowl Aug 5, 2020

Choose a reason for hiding this comment

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

I tried including gcinterface.h but that seems to have other dependencies


COOP_PINVOKE_HELPER(Object *, RhpNewFinalizable, (EEType* pEEType))
{
Expand Down Expand Up @@ -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)
Copy link
Member

@jkotas jkotas Aug 10, 2020

Choose a reason for hiding this comment

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

Use FEATURE_64BIT_ALIGNMENT here?


GPTR_DECL(EEType, g_pFreeObjectEEType);

#ifdef HOST_ARM
COOP_PINVOKE_HELPER(Object *, RhpNewFinalizableAlign8, (EEType* pEEType))
{
Object * pObject = nullptr;
/* TODO */ ASSERT_UNCONDITIONALLY("NYI");
Copy link
Contributor Author

@yowl yowl Aug 5, 2020

Choose a reason for hiding this comment

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

Haven't looked at this one, but if its trivial I could fill it out.

Copy link
Member

@jkotas jkotas Aug 10, 2020

Choose a reason for hiding this comment

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

Yes, this can be pretty simple - similar to RhpNewFastMisalign

Copy link
Contributor Author

@yowl yowl Aug 12, 2020

Choose a reason for hiding this comment

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

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

corert/src/Common/src/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

Line 829 in 127405d

alignment = target.GetObjectAlignment(alignment);
are structs so can't put finalizers on those.

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;
        }
    }

Copy link
Member

@jkotas jkotas Aug 12, 2020

Choose a reason for hiding this comment

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

Good point. You are right - this is de-facto unreachable. It may be worth it to add a comment about it.

return pObject;
}

COOP_PINVOKE_HELPER(Object *, RhpNewFastMisalign, (EEType* pEEType))
COOP_PINVOKE_HELPER(Object *, RhpNewFastAlign8, (EEType* pEEType))
{
Object * pObject = nullptr;
/* TODO */ ASSERT_UNCONDITIONALLY("NYI");
ASSERT(pEEType->RequiresAlign8());
ASSERT(!pEEType->HasFinalizer());

Thread* pCurThread = ThreadStore::GetCurrentThread();
gc_alloc_context* acontext = pCurThread->GetAllocContext();
Object* pObject;

size_t size = pEEType->get_BaseSize();
size = (size + (sizeof(UIntNative) - 1)) & ~(sizeof(UIntNative) - 1);

UInt8* result = acontext->alloc_ptr;

int requiresPadding = ((uint32_t)result) & 7;
if (requiresPadding) size += 12;
UInt8* advance = result + size;
if (advance <= acontext->alloc_limit)
{
acontext->alloc_ptr = advance;
if (requiresPadding)
{
Object* dummy = (Object*)result;
dummy->set_EEType(g_pFreeObjectEEType);
result += 12;
}
pObject = (Object*)result;
pObject->set_EEType(pEEType);

return pObject;
}

pObject = (Object*)RhpGcAlloc(pEEType, GC_ALLOC_ALIGN8, size, NULL);
if (pObject == nullptr)
{
ASSERT_UNCONDITIONALLY("NYI"); // TODO: Throw OOM
}
pObject->set_EEType(pEEType);

if (size >= RH_LARGE_OBJECT_SIZE)
RhpPublishObject(pObject, size);

return pObject;
}

COOP_PINVOKE_HELPER(Object *, RhpNewFastAlign8, (EEType* pEEType))
COOP_PINVOKE_HELPER(Object*, RhpNewFastMisalign, (EEType* pEEType))
{
Object * pObject = nullptr;
/* TODO */ ASSERT_UNCONDITIONALLY("NYI");
size_t size = pEEType->get_BaseSize();
size = (size + 7) & ~7;
Copy link
Member

@jkotas jkotas Aug 10, 2020

Choose a reason for hiding this comment

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

This should not be needed. I do not see it in the asm version either.

Object* pObject = (Object*)RhpGcAlloc(pEEType, GC_ALLOC_ALIGN8_BIAS, size, NULL);
if (pObject == nullptr)
{
ASSERT_UNCONDITIONALLY("NYI"); // TODO: Throw OOM
}
pObject->set_EEType(pEEType);

if (size >= RH_LARGE_OBJECT_SIZE)
RhpPublishObject(pObject, size);

return pObject;
}

COOP_PINVOKE_HELPER(Array *, RhpNewArrayAlign8, (EEType * pArrayEEType, int numElements))
{
Array * pObject = nullptr;
/* TODO */ ASSERT_UNCONDITIONALLY("NYI");
ASSERT_MSG(pArrayEEType->RequiresAlign8(), "RhpNewArrayAlign8 called for a type that is not aligned 8");

Thread* pCurThread = ThreadStore::GetCurrentThread();
gc_alloc_context* acontext = pCurThread->GetAllocContext();
Array* pObject;

if (numElements < 0)
{
ASSERT_UNCONDITIONALLY("NYI"); // TODO: Throw overflow
}

size_t size;

UInt32 baseSize = pArrayEEType->get_BaseSize();
#ifndef HOST_64BIT
// if the element count is <= 0x10000, no overflow is possible because the component size is
// <= 0xffff, and thus the product is <= 0xffff0000, and the base size is only ~12 bytes
if (numElements > 0x10000)
{
// Perform the size computation using 64-bit integeres to detect overflow
uint64_t size64 = (uint64_t)baseSize + ((uint64_t)numElements * (uint64_t)pArrayEEType->get_ComponentSize());
size64 = (size64 + (sizeof(UIntNative) - 1)) & ~(sizeof(UIntNative) - 1);

size = (size_t)size64;
if (size != size64)
{
ASSERT_UNCONDITIONALLY("NYI"); // TODO: Throw overflow
}
}
else
#endif // !HOST_64BIT
{
size = (size_t)baseSize + ((size_t)numElements * (size_t)pArrayEEType->get_ComponentSize());
size = ALIGN_UP(size, sizeof(UIntNative));
}
UInt8* result = acontext->alloc_ptr;
int requiresAlignObject = ((uint32_t)result) & 7;
if (requiresAlignObject) size += 12;

UInt8* advance = result + size;
if (advance <= acontext->alloc_limit)
{
acontext->alloc_ptr = advance;
if (requiresAlignObject)
{
Object* dummy = (Object*)result;
dummy->set_EEType(g_pFreeObjectEEType);
result += 12;
}
pObject = (Array*)result;
pObject->set_EEType(pArrayEEType);
pObject->InitArrayLength((UInt32)numElements);
return pObject;
}

pObject = (Array*)RhpGcAlloc(pArrayEEType, GC_ALLOC_ALIGN8, size, NULL);
if (pObject == nullptr)
{
ASSERT_UNCONDITIONALLY("NYI"); // TODO: Throw OOM
}
pObject->set_EEType(pArrayEEType);
pObject->InitArrayLength((UInt32)numElements);

if (size >= RH_LARGE_OBJECT_SIZE)
RhpPublishObject(pObject, size);

return pObject;
}
#endif
#endif // defined(HOST_ARM) || defined(HOST_WASM)

COOP_PINVOKE_HELPER(void, RhpInitialDynamicInterfaceDispatch, ())
{
Expand Down
8 changes: 4 additions & 4 deletions src/Native/gc/gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17833,12 +17833,12 @@ uint8_t* gc_heap::find_object (uint8_t* interior)
{
// this is a pointer to a UOH object
heap_segment* seg = find_segment (interior, FALSE);
if (seg
if (seg)
{
#ifdef FEATURE_CONSERVATIVE_GC
&& (GCConfig::GetConservativeGC() || interior <= heap_segment_allocated(seg))
if ( interior >= heap_segment_allocated(seg))
Copy link
Contributor Author

@yowl yowl Aug 5, 2020

Choose a reason for hiding this comment

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

This is the same as merged in dotnet/runtime#39905 . If this complicates merging, I can leave this PR as WIP.

return 0;
#endif
)
{
// If interior falls within the first free object at the beginning of a generation,
// we don't have brick entry for it, and we may incorrectly treat it as on large object heap.
int align_const = get_alignment_constant (heap_segment_read_only_p (seg)
Expand Down
3 changes: 3 additions & 0 deletions src/Runtime.Base/src/Runtime.Base.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
<PropertyGroup Condition="'$(Platform)' == 'armel'">
<DefineConstants>FEATURE_64BIT_ALIGNMENT;$(DefineConstants)</DefineConstants>
</PropertyGroup>
<PropertyGroup Condition="'$(Platform)' == 'wasm'">
<DefineConstants>FEATURE_64BIT_ALIGNMENT;$(DefineConstants)</DefineConstants>
</PropertyGroup>

<ItemGroup>
<Compile Include="System\Nullable.cs" />
Expand Down
1 change: 1 addition & 0 deletions src/System.Private.CoreLib/src/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@
<DefineConstants>INPLACE_RUNTIME;$(DefineConstants)</DefineConstants>
<DefineConstants Condition="'$(Platform)' == 'arm'">FEATURE_64BIT_ALIGNMENT;$(DefineConstants)</DefineConstants>
<DefineConstants Condition="'$(Platform)' == 'armel'">FEATURE_64BIT_ALIGNMENT;$(DefineConstants)</DefineConstants>
<DefineConstants Condition="'$(Platform)' == 'wasm'">FEATURE_64BIT_ALIGNMENT;$(DefineConstants)</DefineConstants>
</PropertyGroup>
<ItemGroup Condition="'$(InPlaceRuntime)' == 'true'">
<Compile Include="..\..\Runtime.Base\src\System\Diagnostics\Eval.cs">
Expand Down