Skip to content

Commit

Permalink
Inject IJW Copy Constructor calls in the JIT instead of in the IL stu…
Browse files Browse the repository at this point in the history
…bs (dotnet#106000)

* Revert "Disable GS checks in Reverse P/Invoke stubs to avoid missing these copies."

This reverts commit 79b1cd2.

* Revert "Since the fix is in the CoreCLR VM side, we need to disable the tests in JitStress (as they won't be fixed when the JIT introduces these extra checks when not asking the VM)"

This reverts commit 35d0de9.

* Propagate the modreq to the stub and into the GS cookie logic to avoid creating shadow vars

* Add RuntimeHelpers copy-constructor helper

* Implement Reverse P/Invoke GS shadow copy for copy constructors

* Use a jitinterface function instead of a jit helper so we don't need to do the generic method instantiation on every call (only when jitting)

* Remove extra P/Invoke copy constructor call

* Propagate the "special-copy" modifier into the JIT for byref locals (which the P/Invoke scenario will use)

* Move copy constructor handling into the JIT for arguments

* Remove now-dead code

* Fix formatting

* Fix build on Windows x64

* Call copy helper in lowering and heavily reduce changes in higher layers

* Revert "Call copy helper in lowering and heavily reduce changes in higher layers"

This reverts commit 390185c.

* Implement copy-constructor handling for P/Invokes via a map back to the original arg and don't do intermediate copy-constructor calls.

The bitwise copies are invalid, but there's no possible way for the user to see them. They'll always get passed a correctly copy-constructed value.

* Various PR feedback and propagate special copy directly from the IL arguments to the arg slot instead of tracing through locals.

* Use a simple bool array instead of a vector as a function that has one parameter of this type will likely have multiple.

* Format

* Update hashing

* Remove assert

* Update Misc test

* Cast through size_t to make it explicit that we're dropping the top bits

* Use compArgsCount and remove assert

* Various fixes

* Fix GCC build

* Fix signature type check

* Feedback

* Add headers

* Fix superpmi build failures. Correctly handle multiple-arg copy constructor scenarios (by counting args in the correct direction)

* Add check and assert
  • Loading branch information
jkoritzinsky committed Aug 14, 2024
1 parent 151311a commit ec067c8
Show file tree
Hide file tree
Showing 42 changed files with 1,197 additions and 755 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,12 @@ internal static int EnumCompareTo<T>(T x, T y) where T : struct, Enum
return x.CompareTo(y);
}


// The body of this function will be created by the EE for the specific type.
// See getILIntrinsicImplementation for how this happens.
[Intrinsic]
internal static extern unsafe void CopyConstruct<T>(T* dest, T* src) where T : unmanaged;

internal static ref byte GetRawData(this object obj) =>
ref Unsafe.As<RawData>(obj).Data;

Expand Down
69 changes: 0 additions & 69 deletions src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1315,75 +1315,6 @@ public IntPtr AddRef()
}
} // class CleanupWorkListElement

internal unsafe struct CopyConstructorCookie
{
private void* m_source;

private nuint m_destinationOffset;

public delegate*<void*, void*, void> m_copyConstructor;

public delegate*<void*, void> m_destructor;

public CopyConstructorCookie* m_next;

[StackTraceHidden]
public void ExecuteCopy(void* destinationBase)
{
if (m_copyConstructor != null)
{
m_copyConstructor((byte*)destinationBase + m_destinationOffset, m_source);
}

if (m_destructor != null)
{
m_destructor(m_source);
}
}
}

internal unsafe struct CopyConstructorChain
{
public void* m_realTarget;
public CopyConstructorCookie* m_head;

public void Add(CopyConstructorCookie* cookie)
{
cookie->m_next = m_head;
m_head = cookie;
}

[ThreadStatic]
private static CopyConstructorChain s_copyConstructorChain;

public void Install(void* realTarget)
{
m_realTarget = realTarget;
s_copyConstructorChain = this;
}

[StackTraceHidden]
private void ExecuteCopies(void* destinationBase)
{
for (CopyConstructorCookie* current = m_head; current != null; current = current->m_next)
{
current->ExecuteCopy(destinationBase);
}
}

[UnmanagedCallersOnly]
[StackTraceHidden]
public static void* ExecuteCurrentCopiesAndGetTarget(void* destinationBase)
{
void* target = s_copyConstructorChain.m_realTarget;
s_copyConstructorChain.ExecuteCopies(destinationBase);
// Reset this instance to ensure we don't accidentally execute the copies again.
// All of the pointers point to the stack, so we don't need to free any memory.
s_copyConstructorChain = default;
return target;
}
}

internal static partial class StubHelpers
{
[MethodImpl(MethodImplOptions.InternalCall)]
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,7 @@ enum CorInfoTypeWithMod
{
CORINFO_TYPE_MASK = 0x3F, // lower 6 bits are type mask
CORINFO_TYPE_MOD_PINNED = 0x40, // can be applied to CLASS, or BYREF to indicate pinned
CORINFO_TYPE_MOD_COPY_WITH_HELPER = 0x80 // can be applied to VALUECLASS to indicate 'needs helper to copy'
};

inline CorInfoType strip(CorInfoTypeWithMod val) {
Expand Down Expand Up @@ -3325,6 +3326,8 @@ class ICorDynamicInfo : public ICorStaticInfo
// but for tailcalls, the contract is that JIT leaves the indirection cell in
// a register during tailcall.
virtual void updateEntryPointForTailCall(CORINFO_CONST_LOOKUP* entryPoint) = 0;

virtual CORINFO_METHOD_HANDLE getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0;
};

/**********************************************************************************/
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/inc/icorjitinfoimpl_generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,9 @@ uint32_t getJitFlags(
CORJIT_FLAGS* flags,
uint32_t sizeInBytes) override;

CORINFO_METHOD_HANDLE getSpecialCopyHelper(
CORINFO_CLASS_HANDLE type) override;

/**********************************************************************************/
// clang-format on
/**********************************************************************************/
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID;
#define GUID_DEFINED
#endif // !GUID_DEFINED

constexpr GUID JITEEVersionIdentifier = { /* f43f9022-8795-4791-ba55-c450d76cfeb9 */
0xf43f9022,
0x8795,
0x4791,
{0xba, 0x55, 0xc4, 0x50, 0xd7, 0x6c, 0xfe, 0xb9}
constexpr GUID JITEEVersionIdentifier = { /* 62865a69-7c84-4ba5-8636-a7dec55c05a7 */
0x62865a69,
0x7c84,
0x4ba5,
{0x86, 0x36, 0xa7, 0xde, 0xc5, 0x5c, 0x05, 0xa7}
};

//////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/ICorJitInfo_names_generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,5 +180,6 @@ DEF_CLR_API(recordRelocation)
DEF_CLR_API(getRelocTypeHint)
DEF_CLR_API(getExpectedTargetArchitecture)
DEF_CLR_API(getJitFlags)
DEF_CLR_API(getSpecialCopyHelper)

#undef DEF_CLR_API
9 changes: 9 additions & 0 deletions src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,15 @@ uint32_t WrapICorJitInfo::getJitFlags(
return temp;
}

CORINFO_METHOD_HANDLE WrapICorJitInfo::getSpecialCopyHelper(
CORINFO_CLASS_HANDLE type)
{
API_ENTER(getSpecialCopyHelper);
CORINFO_METHOD_HANDLE temp = wrapHnd->getSpecialCopyHelper(type);
API_LEAVE(getSpecialCopyHelper);
return temp;
}

/**********************************************************************************/
// clang-format on
/**********************************************************************************/
4 changes: 4 additions & 0 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1989,6 +1989,10 @@ void Compiler::compInit(ArenaAllocator* pAlloc,
m_fpStructLoweringCache = nullptr;
#endif

#if defined(TARGET_X86) && defined(FEATURE_IJW)
m_specialCopyArgs = nullptr;
#endif

// check that HelperCallProperties are initialized

assert(s_helperCallProperties.IsPure(CORINFO_HELP_GET_GCSTATIC_BASE));
Expand Down
30 changes: 30 additions & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -5703,6 +5703,36 @@ class Compiler
const CORINFO_SWIFT_LOWERING* GetSwiftLowering(CORINFO_CLASS_HANDLE clsHnd);
#endif

#if defined(TARGET_X86) && defined(FEATURE_IJW)
bool* m_specialCopyArgs;
bool recordArgRequiresSpecialCopy(unsigned argNum)
{
if (argNum >= info.compArgsCount)
{
return false;
}

if (m_specialCopyArgs == nullptr)
{
m_specialCopyArgs = new (getAllocator()) bool[info.compArgsCount];
memset(m_specialCopyArgs, 0, info.compArgsCount * sizeof(bool));
}

m_specialCopyArgs[argNum] = true;
return true;
}

bool argRequiresSpecialCopy(unsigned argNum)
{
return argNum < info.compArgsCount && m_specialCopyArgs != nullptr && m_specialCopyArgs[argNum];
}

bool compHasSpecialCopyArgs()
{
return m_specialCopyArgs != nullptr;
}
#endif

void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN);
void optCopyLoopMemoryDependence(GenTree* fromTree, GenTree* toTree);

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16599,6 +16599,7 @@ GenTree* Compiler::gtNewTempStore(
valTyp = lvaGetRealType(val->AsLclVar()->GetLclNum());
val->gtType = valTyp;
}

var_types dstTyp = varDsc->TypeGet();

/* If the variable's lvType is not yet set then set it here */
Expand Down
58 changes: 52 additions & 6 deletions src/coreclr/jit/gschecks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -516,13 +516,59 @@ void Compiler::gsParamsToShadows()
continue;
}

GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet());
src->gtFlags |= GTF_DONT_CSE;
GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src);
#if defined(TARGET_X86) && defined(FEATURE_IJW)
if (lclNum < info.compArgsCount && argRequiresSpecialCopy(lclNum) && (varDsc->TypeGet() == TYP_STRUCT))
{
JITDUMP("arg%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum,
shadowVarNum);
CORINFO_METHOD_HANDLE copyHelper =
info.compCompHnd->getSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle());
GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID);

GenTree* src = gtNewLclVarAddrNode(lclNum);
GenTree* dst = gtNewLclVarAddrNode(shadowVarNum);

call->gtArgs.PushBack(this, NewCallArg::Primitive(dst));
call->gtArgs.PushBack(this, NewCallArg::Primitive(src));

fgEnsureFirstBBisScratch();
compCurBB = fgFirstBB; // Needed by some morphing
if (opts.IsReversePInvoke())
{
JITDUMP(
"Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n");

#ifdef DEBUG
// assert that we don't have any uses of the local variable in the first block
// before we insert the shadow copy statement.
for (Statement* const stmt : fgFirstBB->Statements())
{
assert(!gtHasRef(stmt->GetRootNode(), lclNum));
}
#endif
// If we are in a reverse P/Invoke, then we need to insert
// the call at the end of the first block as we need to do the GC transition
// before we can call the helper.
(void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(call));
}
else
{
JITDUMP("Inserting special copy helper call at the beginning of the first block\n");
(void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(call));
}
}
else
#endif // TARGET_X86 && FEATURE_IJW
{
GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet());
src->gtFlags |= GTF_DONT_CSE;

fgEnsureFirstBBisScratch();
compCurBB = fgFirstBB; // Needed by some morphing
(void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store));
GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src);

fgEnsureFirstBBisScratch();
compCurBB = fgFirstBB; // Needed by some morphing
(void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store));
}
}
compCurBB = nullptr;

Expand Down
13 changes: 13 additions & 0 deletions src/coreclr/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,19 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un
CorInfoTypeWithMod corInfoType = info.compCompHnd->getArgType(&info.compMethodInfo->args, argLst, &typeHnd);
varDsc->lvIsParam = 1;

#if defined(TARGET_X86) && defined(FEATURE_IJW)
if ((corInfoType & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0)
{
CorInfoType typeWithoutMod = strip(corInfoType);
if (typeWithoutMod == CORINFO_TYPE_VALUECLASS || typeWithoutMod == CORINFO_TYPE_PTR ||
typeWithoutMod == CORINFO_TYPE_BYREF)
{
JITDUMP("Marking user arg%02u as requiring special copy semantics\n", i);
recordArgRequiresSpecialCopy(i);
}
}
#endif // TARGET_X86 && FEATURE_IJW

lvaInitVarDsc(varDsc, varDscInfo->varNum, strip(corInfoType), typeHnd, argLst, &info.compMethodInfo->args);

if (strip(corInfoType) == CORINFO_TYPE_CLASS)
Expand Down
Loading

0 comments on commit ec067c8

Please sign in to comment.