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

Convert MemoryMarshal.GetArrayDataReference to a JIT intrinsic #72725

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f1dc80f
Convert MemoryMarshal.GetArrayDataReference to a JIT intrinsic
MichalPetryka Jul 23, 2022
7c01508
Update importer.cpp
MichalPetryka Jul 23, 2022
46776a9
Update importer.cpp
MichalPetryka Jul 23, 2022
51e0ebf
Update importer.cpp
MichalPetryka Jul 23, 2022
4dab103
Update MemoryMarshalGetArrayDataReference.cs
MichalPetryka Jul 23, 2022
bb35116
Update MemoryMarshalGetArrayDataReference.cs
MichalPetryka Jul 23, 2022
3d06976
Fix nullchecks, improve tests, remove dead code
MichalPetryka Jul 24, 2022
daafd55
Update project files
MichalPetryka Jul 24, 2022
ae1ef3d
Update MemoryMarshalGetArrayDataReference.cs
MichalPetryka Jul 24, 2022
039eb72
Fix cloning of bounds-check-less INDEX_ADDRs
SingleAccretion Jul 24, 2022
2405b2b
Merge remote-tracking branch 'singleaccertion/Fix-IndexAddr-BoundsChe…
MichalPetryka Jul 24, 2022
10ee80c
Fix formatting
MichalPetryka Jul 24, 2022
b5de179
Remove COMMA
MichalPetryka Jul 25, 2022
5aa3e50
Move the nullcheck insertion to morph
MichalPetryka Jul 25, 2022
c14698a
Fix compilation
MichalPetryka Jul 25, 2022
b7e6d4e
Revert morph changes
MichalPetryka Jul 26, 2022
7af6e18
Try a hack to see if the diffs are better
MichalPetryka Jul 26, 2022
0a92a64
Revert "Try a hack to see if the diffs are better"
MichalPetryka Jul 26, 2022
88ee12d
Add more tests
MichalPetryka Jul 26, 2022
0cf5302
Future-proof against delegate inlining
MichalPetryka Jul 26, 2022
fe114d5
Merge remote-tracking branch 'original/main' into getarraydatareferen…
MichalPetryka Jul 27, 2022
8c16206
Merge remote-tracking branch 'original/main' into getarraydatareferen…
MichalPetryka Aug 3, 2022
4e80af1
Merge remote-tracking branch 'original/main' into getarraydatareferen…
MichalPetryka Aug 27, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ public static unsafe partial class MemoryMarshal
/// </remarks>
[Intrinsic]
[NonVersionable]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ref T GetArrayDataReference<T>(T[] array) =>
ref Unsafe.As<byte, T>(ref Unsafe.As<RawArrayData>(array).Data);
ref GetArrayDataReference(array);

/// <summary>
/// Returns a reference to the 0th element of <paramref name="array"/>. If the array is empty, returns a reference to where the 0th element
Expand Down
13 changes: 10 additions & 3 deletions src/coreclr/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -910,7 +910,7 @@ inline GenTreeIntCon* Compiler::gtNewIconHandleNode(size_t value, GenTreeFlags f
#if defined(LATE_DISASM)
node = new (this, LargeOpOpcode()) GenTreeIntCon(TYP_I_IMPL, value, fields DEBUGARG(/*largeNode*/ true));
#else
node = new (this, GT_CNS_INT) GenTreeIntCon(TYP_I_IMPL, value, fields);
node = new (this, GT_CNS_INT) GenTreeIntCon(TYP_I_IMPL, value, fields);
#endif
node->gtFlags |= flags;
return node;
Expand Down Expand Up @@ -1146,8 +1146,15 @@ inline GenTreeIndexAddr* Compiler::gtNewIndexAddr(GenTree* arrayOp,
unsigned elemSize =
(elemType == TYP_STRUCT) ? info.compCompHnd->getClassSize(elemClassHandle) : genTypeSize(elemType);

GenTreeIndexAddr* indexAddr = new (this, GT_INDEX_ADDR)
GenTreeIndexAddr(arrayOp, indexOp, elemType, elemClassHandle, elemSize, lengthOffset, firstElemOffset);
#ifdef DEBUG
bool boundsCheck = JitConfig.JitSkipArrayBoundCheck() != 1;
#else
bool boundsCheck = true;
#endif

GenTreeIndexAddr* indexAddr =
new (this, GT_INDEX_ADDR) GenTreeIndexAddr(arrayOp, indexOp, elemType, elemClassHandle, elemSize, lengthOffset,
firstElemOffset, boundsCheck);

return indexAddr;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8466,7 +8466,7 @@ GenTree* Compiler::gtCloneExpr(
copy = new (this, GT_INDEX_ADDR)
GenTreeIndexAddr(asIndAddr->Arr(), asIndAddr->Index(), asIndAddr->gtElemType,
asIndAddr->gtStructElemClass, asIndAddr->gtElemSize, asIndAddr->gtLenOffset,
asIndAddr->gtElemOffset);
asIndAddr->gtElemOffset, asIndAddr->IsBoundsChecked());
copy->AsIndexAddr()->gtIndRngFailBB = asIndAddr->gtIndRngFailBB;
}
break;
Expand Down
12 changes: 4 additions & 8 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -6389,7 +6389,8 @@ struct GenTreeIndexAddr : public GenTreeOp
CORINFO_CLASS_HANDLE structElemClass,
unsigned elemSize,
unsigned lenOffset,
unsigned elemOffset)
unsigned elemOffset,
bool boundsCheck)
: GenTreeOp(GT_INDEX_ADDR, TYP_BYREF, arr, ind)
, gtStructElemClass(structElemClass)
, gtIndRngFailBB(nullptr)
Expand All @@ -6399,13 +6400,8 @@ struct GenTreeIndexAddr : public GenTreeOp
, gtElemOffset(elemOffset)
{
assert(!varTypeIsStruct(elemType) || (structElemClass != NO_CLASS_HANDLE));
#ifdef DEBUG
if (JitConfig.JitSkipArrayBoundCheck() == 1)
{
// Skip bounds check
}
else
#endif

if (boundsCheck)
{
// Do bounds check
gtFlags |= GTF_INX_RNGCHK;
Expand Down
33 changes: 33 additions & 0 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3879,6 +3879,29 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
break;
}

case NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference:
{
assert(sig->numArgs == 1);

GenTree* array = impPopStack().val;
CORINFO_CLASS_HANDLE elemHnd = sig->sigInst.methInst[0];
CorInfoType jitType = info.compCompHnd->asCorInfoType(elemHnd);
var_types elemType = JITtype2varType(jitType);

GenTree* arrayClone;
array = impCloneExpr(array, &arrayClone, NO_CLASS_HANDLE, (unsigned)CHECK_SPILL_ALL,
nullptr DEBUGARG("MemoryMarshal.GetArrayDataReference array"));

impAppendTree(gtNewNullCheck(array, compCurBB), (unsigned)CHECK_SPILL_ALL, impCurStmtDI);

GenTree* index = gtNewIconNode(0, TYP_I_IMPL);
GenTreeIndexAddr* indexAddr = gtNewArrayIndexAddr(arrayClone, index, elemType, elemHnd);
indexAddr->gtFlags &= ~GTF_INX_RNGCHK;
MichalPetryka marked this conversation as resolved.
Show resolved Hide resolved
indexAddr->gtFlags |= GTF_INX_ADDR_NONNULL;
retNode = indexAddr;
break;
}

case NI_Internal_Runtime_MethodTable_Of:
case NI_System_Activator_AllocatorOf:
case NI_System_Activator_DefaultConstructorOf:
Expand Down Expand Up @@ -6090,6 +6113,16 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
}
}
}
else if (strcmp(namespaceName, "System.Runtime.InteropServices") == 0)
{
if (strcmp(className, "MemoryMarshal") == 0)
{
if (strcmp(methodName, "GetArrayDataReference") == 0)
{
result = NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference;
}
}
}
else if (strncmp(namespaceName, "System.Runtime.Intrinsics", 25) == 0)
{
// We go down this path even when FEATURE_HW_INTRINSICS isn't enabled
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/namedintrinsiclist.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ enum NamedIntrinsic : unsigned short
NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray,
NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant,

NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference,

NI_System_String_Equals,
NI_System_String_get_Chars,
NI_System_String_get_Length,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ public static unsafe partial class MemoryMarshal
/// </remarks>
[Intrinsic]
[NonVersionable]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ref T GetArrayDataReference<T>(T[] array) =>
ref Unsafe.As<byte, T>(ref Unsafe.As<RawArrayData>(array).Data);
ref GetArrayDataReference(array);

/// <summary>
/// Returns a reference to the 0th element of <paramref name="array"/>. If the array is empty, returns a reference to where the 0th element
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,6 @@ private MethodIL TryGetIntrinsicMethodIL(MethodDesc method)
return UnsafeIntrinsics.EmitIL(method);
}
break;
case "MemoryMarshal":
{
if (owningType.Namespace == "System.Runtime.InteropServices")
return MemoryMarshalIntrinsics.EmitIL(method);
}
break;
case "Volatile":
{
if (owningType.Namespace == "System.Threading")
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -649,9 +649,6 @@
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\UnsafeIntrinsics.cs">
<Link>IL\Stubs\UnsafeIntrinsics.cs</Link>
</Compile>
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\MemoryMarshalIntrinsics.cs">
<Link>IL\Stubs\MemoryMarshalIntrinsics.cs</Link>
</Compile>
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\VolatileIntrinsics.cs">
<Link>IL\Stubs\VolatileIntrinsics.cs</Link>
</Compile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,6 @@ private MethodIL TryGetIntrinsicMethodIL(MethodDesc method)
return UnsafeIntrinsics.EmitIL(method);
}

if (mdType.Name == "MemoryMarshal" && mdType.Namespace == "System.Runtime.InteropServices")
{
return MemoryMarshalIntrinsics.EmitIL(method);
}

if (mdType.Name == "Volatile" && mdType.Namespace == "System.Threading")
{
return VolatileIntrinsics.EmitIL(method);
Expand Down Expand Up @@ -171,7 +166,7 @@ public override MethodIL GetMethodIL(MethodDesc method)
}

// Check to see if there is an override for the EcmaMethodIL. If there is not
// then simply return the EcmaMethodIL. In theory this could call
// then simply return the EcmaMethodIL. In theory this could call
// CreateCrossModuleInlineableTokensForILBody, but we explicitly do not want
// to do that. The reason is that this method is called during the multithreaded
// portion of compilation, and CreateCrossModuleInlineableTokensForILBody
Expand Down Expand Up @@ -224,7 +219,7 @@ class ManifestModuleWrappedMethodIL : MethodIL, IEcmaMethodIL, IMethodTokensAreU
MutableModule _mutableModule;

public ManifestModuleWrappedMethodIL() {}

public bool Initialize(MutableModule mutableModule, EcmaMethodIL wrappedMethod)
{
bool failedToReplaceToken = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
<Compile Include="..\..\Common\TypeSystem\IL\ILReader.cs" Link="IL\ILReader.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\ComparerIntrinsics.cs" Link="IL\Stubs\ComparerIntrinsics.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\InterlockedIntrinsics.cs" Link="IL\Stubs\InterlockedIntrinsics.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\MemoryMarshalIntrinsics.cs" Link="IL\Stubs\MemoryMarshalIntrinsics.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\RuntimeHelpersIntrinsics.cs" Link="IL\Stubs\RuntimeHelpersIntrinsics.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\UnsafeIntrinsics.cs" Link="IL\Stubs\UnsafeIntrinsics.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\VolatileIntrinsics.cs" Link="IL\Stubs\VolatileIntrinsics.cs" />
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/corelib.h
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,6 @@ DEFINE_METHOD(UNSAFE, UNBOX, Unbox, NoSig)
DEFINE_METHOD(UNSAFE, WRITE, Write, NoSig)

DEFINE_CLASS(MEMORY_MARSHAL, Interop, MemoryMarshal)
DEFINE_METHOD(MEMORY_MARSHAL, GET_ARRAY_DATA_REFERENCE_SZARRAY, GetArrayDataReference, GM_ArrT_RetRefT)
DEFINE_METHOD(MEMORY_MARSHAL, GET_ARRAY_DATA_REFERENCE_MDARRAY, GetArrayDataReference, SM_Array_RetRefByte)

DEFINE_CLASS(INTERLOCKED, Threading, Interlocked)
Expand Down
37 changes: 0 additions & 37 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6975,39 +6975,6 @@ bool getILIntrinsicImplementationForUnsafe(MethodDesc * ftn,
return false;
}

bool getILIntrinsicImplementationForMemoryMarshal(MethodDesc * ftn,
MichalPetryka marked this conversation as resolved.
Show resolved Hide resolved
CORINFO_METHOD_INFO * methInfo)
{
STANDARD_VM_CONTRACT;

_ASSERTE(CoreLibBinder::IsClass(ftn->GetMethodTable(), CLASS__MEMORY_MARSHAL));

mdMethodDef tk = ftn->GetMemberDef();

if (tk == CoreLibBinder::GetMethod(METHOD__MEMORY_MARSHAL__GET_ARRAY_DATA_REFERENCE_SZARRAY)->GetMemberDef())
{
mdToken tokRawSzArrayData = CoreLibBinder::GetField(FIELD__RAW_ARRAY_DATA__DATA)->GetMemberDef();

static BYTE ilcode[] = { CEE_LDARG_0,
CEE_LDFLDA,0,0,0,0,
CEE_RET };

ilcode[2] = (BYTE)(tokRawSzArrayData);
ilcode[3] = (BYTE)(tokRawSzArrayData >> 8);
ilcode[4] = (BYTE)(tokRawSzArrayData >> 16);
ilcode[5] = (BYTE)(tokRawSzArrayData >> 24);

methInfo->ILCode = const_cast<BYTE*>(ilcode);
methInfo->ILCodeSize = sizeof(ilcode);
methInfo->maxStack = 1;
methInfo->EHcount = 0;
methInfo->options = (CorInfoOptions)0;
return true;
}

return false;
}

bool getILIntrinsicImplementationForVolatile(MethodDesc * ftn,
CORINFO_METHOD_INFO * methInfo)
{
Expand Down Expand Up @@ -7438,10 +7405,6 @@ getMethodInfoHelper(
{
fILIntrinsic = getILIntrinsicImplementationForUnsafe(ftn, methInfo);
}
else if (CoreLibBinder::IsClass(pMT, CLASS__MEMORY_MARSHAL))
{
fILIntrinsic = getILIntrinsicImplementationForMemoryMarshal(ftn, methInfo);
}
else if (CoreLibBinder::IsClass(pMT, CLASS__INTERLOCKED))
{
fILIntrinsic = getILIntrinsicImplementationForInterlocked(ftn, methInfo);
Expand Down
Loading