Skip to content

Commit

Permalink
Fix conditions for implicit use of hardware intrinsics (#45662)
Browse files Browse the repository at this point in the history
JIT uses hardware intrinsics implicitly in certain cases. Unlike regular hardware intrinsics, these
transformations are not guarded using explicit IsSupported checks. NativeAOT had fragile code that tried
to step around this limitation. This change makes this contract explicit by allowing notifyInstructionSetUsage
callback to return bool that the EE side can use to suppress implicit use of hardware intrinsics.

Fixes dotnet/runtimelab#425
  • Loading branch information
jkotas authored Dec 8, 2020
1 parent cbc10fa commit 518eed2
Show file tree
Hide file tree
Showing 20 changed files with 86 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ bool convertPInvokeCalliToCall(
CORINFO_RESOLVED_TOKEN* pResolvedToken,
bool mustConvert);

void notifyInstructionSetUsage(
bool notifyInstructionSetUsage(
CORINFO_InstructionSet instructionSet,
bool supportEnabled);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2105,7 +2105,7 @@ DWORD interceptor_ICJI::getExpectedTargetArchitecture()
return original_ICorJitInfo->getExpectedTargetArchitecture();
}

void interceptor_ICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet, bool supported)
bool interceptor_ICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet, bool supported)
{
original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supported);
return original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supported);
}
Original file line number Diff line number Diff line change
Expand Up @@ -1225,12 +1225,12 @@ bool interceptor_ICJI::convertPInvokeCalliToCall(
return original_ICorJitInfo->convertPInvokeCalliToCall(pResolvedToken, mustConvert);
}

void interceptor_ICJI::notifyInstructionSetUsage(
bool interceptor_ICJI::notifyInstructionSetUsage(
CORINFO_InstructionSet instructionSet,
bool supportEnabled)
{
mcs->AddCall("notifyInstructionSetUsage");
original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supportEnabled);
return original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supportEnabled);
}

void interceptor_ICJI::allocMem(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1072,11 +1072,11 @@ bool interceptor_ICJI::convertPInvokeCalliToCall(
return original_ICorJitInfo->convertPInvokeCalliToCall(pResolvedToken, mustConvert);
}

void interceptor_ICJI::notifyInstructionSetUsage(
bool interceptor_ICJI::notifyInstructionSetUsage(
CORINFO_InstructionSet instructionSet,
bool supportEnabled)
{
original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supportEnabled);
return original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supportEnabled);
}

void interceptor_ICJI::allocMem(
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/ToolBox/superpmi/superpmi/icorjitinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1548,9 +1548,10 @@ bool MyICJI::convertPInvokeCalliToCall(CORINFO_RESOLVED_TOKEN* pResolvedToken, b
return jitInstance->mc->repConvertPInvokeCalliToCall(pResolvedToken, fMustConvert);
}

void MyICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet, bool supported)
bool MyICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet, bool supported)
{
jitInstance->mc->cr->AddCall("notifyInstructionSetUsage");
return supported;
}

// Stuff directly on ICorJitInfo
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -3111,7 +3111,8 @@ class ICorDynamicInfo : public ICorStaticInfo
bool fMustConvert
) = 0;

virtual void notifyInstructionSetUsage(
// Notify EE about intent to use or not to use instruction set in the method. Returns true if the instruction set is supported unconditionally.
virtual bool notifyInstructionSetUsage(
CORINFO_InstructionSet instructionSet,
bool supportEnabled
) = 0;
Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@
//
//////////////////////////////////////////////////////////////////////////////////////////////////////////

constexpr GUID JITEEVersionIdentifier = { /* f4746286-33ad-45a2-a37e-d226d689524d */
0xf4746286,
0x33ad,
0x45a2,
{0xa3, 0x7e, 0xd2, 0x26, 0xd6, 0x89, 0x52, 0x4d}
constexpr GUID JITEEVersionIdentifier = { /* 790de1e5-1426-4ecf-939c-2cd60445c219 */
0x790de1e5,
0x1426,
0x4ecf,
{ 0x93, 0x9c, 0x2c, 0xd6, 0x4, 0x45, 0xc2, 0x19 }
};

//////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/ICorJitInfo_API_wrapper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1495,13 +1495,14 @@ bool WrapICorJitInfo::convertPInvokeCalliToCall(
return temp;
}

void WrapICorJitInfo::notifyInstructionSetUsage(
bool WrapICorJitInfo::notifyInstructionSetUsage(
CORINFO_InstructionSet instructionSet,
bool supportEnabled)
{
API_ENTER(notifyInstructionSetUsage);
wrapHnd->notifyInstructionSetUsage(instructionSet, supportEnabled);
bool temp = wrapHnd->notifyInstructionSetUsage(instructionSet, supportEnabled);
API_LEAVE(notifyInstructionSetUsage);
return temp;
}

void WrapICorJitInfo::allocMem(
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2335,6 +2335,7 @@ void Compiler::compSetProcessor()
CORINFO_InstructionSetFlags instructionSetFlags = jitFlags.GetInstructionSetFlags();
opts.compSupportsISA = 0;
opts.compSupportsISAReported = 0;
opts.compSupportsISAExactly = 0;

#ifdef TARGET_XARCH
if (JitConfig.EnableHWIntrinsic())
Expand Down Expand Up @@ -2504,11 +2505,11 @@ void Compiler::compSetProcessor()
#endif // TARGET_XARCH
}

void Compiler::notifyInstructionSetUsage(CORINFO_InstructionSet isa, bool supported) const
bool Compiler::notifyInstructionSetUsage(CORINFO_InstructionSet isa, bool supported) const
{
const char* isaString = InstructionSetToString(isa);
JITDUMP("Notify VM instruction set (%s) %s be supported.\n", isaString, supported ? "must" : "must not");
info.compCompHnd->notifyInstructionSetUsage(isa, supported);
return info.compCompHnd->notifyInstructionSetUsage(isa, supported);
}

#ifdef PROFILING_SUPPORTED
Expand Down
42 changes: 32 additions & 10 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -8665,24 +8665,22 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
}
#endif // DEBUG

void notifyInstructionSetUsage(CORINFO_InstructionSet isa, bool supported) const;
bool notifyInstructionSetUsage(CORINFO_InstructionSet isa, bool supported) const;

// Answer the question: Is a particular ISA supported?
// Answer the question: Is a particular ISA allowed to be used implicitly by optimizations?
// The result of this api call will exactly match the target machine
// on which the function is executed (except for CoreLib, where there are special rules)
bool compExactlyDependsOn(CORINFO_InstructionSet isa) const
{

#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
uint64_t isaBit = (1ULL << isa);
bool isaSupported = (opts.compSupportsISA & (1ULL << isa)) != 0;
uint64_t isaBit = (1ULL << isa);
if ((opts.compSupportsISAReported & isaBit) == 0)
{
notifyInstructionSetUsage(isa, isaSupported);
if (notifyInstructionSetUsage(isa, (opts.compSupportsISA & isaBit) != 0))
((Compiler*)this)->opts.compSupportsISAExactly |= isaBit;
((Compiler*)this)->opts.compSupportsISAReported |= isaBit;
}

return isaSupported;
return (opts.compSupportsISAExactly & isaBit) != 0;
#else
return false;
#endif
Expand All @@ -8699,7 +8697,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
assert(!isaUseable);
}

// Answer the question: Is a particular ISA supported?
// Answer the question: Is a particular ISA allowed to be used implicitly by optimizations?
// The result of this api call will match the target machine if the result is true
// If the result is false, then the target machine may have support for the instruction
bool compOpportunisticallyDependsOn(CORINFO_InstructionSet isa) const
Expand All @@ -8714,6 +8712,21 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
}
}

// Answer the question: Is a particular ISA supported for explicit hardware intrinsics?
bool compHWIntrinsicDependsOn(CORINFO_InstructionSet isa) const
{
if ((opts.compSupportsISA & (1ULL << isa)) != 0)
{
// Report intent to use the ISA to the EE
compExactlyDependsOn(isa);
return true;
}
else
{
return false;
}
}

bool canUseVexEncoding() const
{
#ifdef TARGET_XARCH
Expand Down Expand Up @@ -8813,8 +8826,17 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
{
JitFlags* jitFlags; // all flags passed from the EE

// The instruction sets that the compiler is allowed to emit.
uint64_t compSupportsISA;
// The instruction sets that were reported to the VM as being used by the current method. Subset of
// compSupportsISA.
uint64_t compSupportsISAReported;
// The instruction sets that the compiler is allowed to take advantage of implicitly during optimizations.
// Subset of compSupportsISA.
// The instruction sets available in compSupportsISA and not available in compSupportsISAExactly can be only
// used via explicit hardware intrinsics.
uint64_t compSupportsISAExactly;

void setSupportedISAs(CORINFO_InstructionSetFlags isas)
{
compSupportsISA = isas.GetFlagsRaw();
Expand Down Expand Up @@ -8913,7 +8935,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#endif

// true if we should use the PINVOKE_{BEGIN,END} helpers instead of generating
// PInvoke transitions inline (e.g. when targeting CoreRT).
// PInvoke transitions inline.
bool ShouldUsePInvokeHelpers()
{
return jitFlags->IsSet(JitFlags::JIT_FLAG_USE_PINVOKE_HELPERS);
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/jit/hwintrinsic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,11 +273,12 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler* comp,
return NI_Illegal;
}

bool isIsaSupported = comp->compExactlyDependsOn(isa) && comp->compSupportsHWIntrinsic(isa);
bool isIsaSupported = comp->compHWIntrinsicDependsOn(isa) && comp->compSupportsHWIntrinsic(isa);

if (strcmp(methodName, "get_IsSupported") == 0)
{
return isIsaSupported ? NI_IsSupported_True : NI_IsSupported_False;
return isIsaSupported ? (comp->compExactlyDependsOn(isa) ? NI_IsSupported_True : NI_IsSupported_Dynamic)
: NI_IsSupported_False;
}
else if (!isIsaSupported)
{
Expand Down Expand Up @@ -606,13 +607,13 @@ GenTree* Compiler::addRangeCheckIfNeeded(
}

//------------------------------------------------------------------------
// compSupportsHWIntrinsic: check whether a given instruction set is supported
// compSupportsHWIntrinsic: check whether a given instruction is enabled via configuration
//
// Arguments:
// isa - Instruction set
//
// Return Value:
// true iff the given instruction set is supported in the current compilation.
// true iff the given instruction set is enabled via configuration (environment variables, etc.).
bool Compiler::compSupportsHWIntrinsic(CORINFO_InstructionSet isa)
{
return JitConfig.EnableHWIntrinsic() && (featureSIMD || HWIntrinsicInfo::isScalarIsa(isa)) &&
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/namedintrinsiclist.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ enum NamedIntrinsic : unsigned short

NI_IsSupported_True,
NI_IsSupported_False,
NI_IsSupported_Dynamic,
NI_Throw_PlatformNotSupportedException,

#ifdef FEATURE_HW_INTRINSICS
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/tools/Common/JitInterface/CorInfoBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2272,16 +2272,17 @@ static byte _convertPInvokeCalliToCall(IntPtr thisHandle, IntPtr* ppException, C
}

[UnmanagedCallersOnly]
static void _notifyInstructionSetUsage(IntPtr thisHandle, IntPtr* ppException, InstructionSet instructionSet, byte supportEnabled)
static byte _notifyInstructionSetUsage(IntPtr thisHandle, IntPtr* ppException, InstructionSet instructionSet, byte supportEnabled)
{
var _this = GetThis(thisHandle);
try
{
_this.notifyInstructionSetUsage(instructionSet, supportEnabled != 0);
return _this.notifyInstructionSetUsage(instructionSet, supportEnabled != 0) ? 1 : 0;
}
catch (Exception ex)
{
*ppException = _this.AllocException(ex);
return default;
}
}

Expand Down Expand Up @@ -2690,7 +2691,7 @@ static IntPtr GetUnmanagedCallbacks()
callbacks[150] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_METHOD_STRUCT_*, void>)&_MethodCompileComplete;
callbacks[151] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_RESOLVED_TOKEN*, CORINFO_SIG_INFO*, CORINFO_GET_TAILCALL_HELPERS_FLAGS, CORINFO_TAILCALL_HELPERS*, byte>)&_getTailCallHelpers;
callbacks[152] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_RESOLVED_TOKEN*, byte, byte>)&_convertPInvokeCalliToCall;
callbacks[153] = (delegate* unmanaged<IntPtr, IntPtr*, InstructionSet, byte, void>)&_notifyInstructionSetUsage;
callbacks[153] = (delegate* unmanaged<IntPtr, IntPtr*, InstructionSet, byte, byte>)&_notifyInstructionSetUsage;
callbacks[154] = (delegate* unmanaged<IntPtr, IntPtr*, uint, uint, uint, uint, CorJitAllocMemFlag, void**, void**, void**, void>)&_allocMem;
callbacks[155] = (delegate* unmanaged<IntPtr, IntPtr*, byte, byte, uint, void>)&_reserveUnwindInfo;
callbacks[156] = (delegate* unmanaged<IntPtr, IntPtr*, byte*, byte*, uint, uint, uint, byte*, CorJitFuncKind, void>)&_allocUnwindInfo;
Expand Down
26 changes: 11 additions & 15 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -394,9 +394,9 @@ private void CompileMethodCleanup()
#if READYTORUN
_profileDataNode = null;
_inlinedMethods = new ArrayBuilder<MethodDesc>();
#endif
_actualInstructionSetSupported = default(InstructionSetFlags);
_actualInstructionSetUnsupported = default(InstructionSetFlags);
#endif
}

private Dictionary<Object, IntPtr> _objectToHandle = new Dictionary<Object, IntPtr>();
Expand Down Expand Up @@ -843,17 +843,7 @@ private uint getMethodAttribsInternal(MethodDesc method)
// Check for hardware intrinsics
if (HardwareIntrinsicHelpers.IsHardwareIntrinsic(method))
{
#if !READYTORUN
// Do not report the get_IsSupported method as an intrinsic - RyuJIT would expand it to
// a constant depending on the code generation flags passed to it, but we would like to
// do a dynamic check instead.
if (
!HardwareIntrinsicHelpers.IsIsSupportedMethod(method)
|| !_compilation.IsHardwareIntrinsicWithRuntimeDeterminedSupport(method))
#endif
{
result |= CorInfoFlag.CORINFO_FLG_JIT_INTRINSIC;
}
result |= CorInfoFlag.CORINFO_FLG_JIT_INTRINSIC;
}

return (uint)result;
Expand Down Expand Up @@ -3228,26 +3218,32 @@ private uint getJitFlags(ref CORJIT_FLAGS flags, uint sizeInBytes)
}


#if READYTORUN
InstructionSetFlags _actualInstructionSetSupported;
InstructionSetFlags _actualInstructionSetUnsupported;

private void notifyInstructionSetUsage(InstructionSet instructionSet, bool supportEnabled)
private bool notifyInstructionSetUsage(InstructionSet instructionSet, bool supportEnabled)
{
if (supportEnabled)
{
_actualInstructionSetSupported.AddInstructionSet(instructionSet);
}
else
{
#if READYTORUN
// By policy we code review all changes into corelib, such that failing to use an instruction
// set is not a reason to not support usage of it.
if (!isMethodDefinedInCoreLib())
#endif
{
_actualInstructionSetUnsupported.AddInstructionSet(instructionSet);
}
}
return supportEnabled;
}
#else
private bool notifyInstructionSetUsage(InstructionSet instructionSet, bool supportEnabled)
{
return supportEnabled ? _compilation.InstructionSetSupport.IsInstructionSetSupported(instructionSet) : false;
}
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ FUNCTIONS
void MethodCompileComplete(CORINFO_METHOD_HANDLE methHnd);
bool getTailCallHelpers(CORINFO_RESOLVED_TOKEN* callToken, CORINFO_SIG_INFO* sig, CORINFO_GET_TAILCALL_HELPERS_FLAGS flags, CORINFO_TAILCALL_HELPERS* pResult);
bool convertPInvokeCalliToCall(CORINFO_RESOLVED_TOKEN * pResolvedToken, bool mustConvert);
void notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet,bool supportEnabled);
bool notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet,bool supportEnabled);
void allocMem( ULONG hotCodeSize, ULONG coldCodeSize, ULONG roDataSize, ULONG xcptnsCount, CorJitAllocMemFlag flag, void** hotCodeBlock, void** coldCodeBlock, void** roDataBlock );
void reserveUnwindInfo(bool isFunclet, bool isColdCode, ULONG unwindSize)
void allocUnwindInfo(BYTE* pHotCode, BYTE* pColdCode, ULONG startOffset, ULONG endOffset, ULONG unwindSize, BYTE* pUnwindBlock, CorJitFuncKind funcKind)
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/tools/aot/jitinterface/jitinterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ struct JitInterfaceCallbacks
void (* MethodCompileComplete)(void * thisHandle, CorInfoExceptionClass** ppException, void* methHnd);
bool (* getTailCallHelpers)(void * thisHandle, CorInfoExceptionClass** ppException, void* callToken, void* sig, int flags, void* pResult);
bool (* convertPInvokeCalliToCall)(void * thisHandle, CorInfoExceptionClass** ppException, void* pResolvedToken, bool mustConvert);
void (* notifyInstructionSetUsage)(void * thisHandle, CorInfoExceptionClass** ppException, int instructionSet, bool supportEnabled);
bool (* notifyInstructionSetUsage)(void * thisHandle, CorInfoExceptionClass** ppException, int instructionSet, bool supportEnabled);
void (* allocMem)(void * thisHandle, CorInfoExceptionClass** ppException, unsigned int hotCodeSize, unsigned int coldCodeSize, unsigned int roDataSize, unsigned int xcptnsCount, int flag, void** hotCodeBlock, void** coldCodeBlock, void** roDataBlock);
void (* reserveUnwindInfo)(void * thisHandle, CorInfoExceptionClass** ppException, bool isFunclet, bool isColdCode, unsigned int unwindSize);
void (* allocUnwindInfo)(void * thisHandle, CorInfoExceptionClass** ppException, unsigned char* pHotCode, unsigned char* pColdCode, unsigned int startOffset, unsigned int endOffset, unsigned int unwindSize, unsigned char* pUnwindBlock, int funcKind);
Expand Down Expand Up @@ -1662,13 +1662,14 @@ class JitInterfaceWrapper
return temp;
}

virtual void notifyInstructionSetUsage(
virtual bool notifyInstructionSetUsage(
int instructionSet,
bool supportEnabled)
{
CorInfoExceptionClass* pException = nullptr;
_callbacks->notifyInstructionSetUsage(_thisHandle, &pException, instructionSet, supportEnabled);
bool temp = _callbacks->notifyInstructionSetUsage(_thisHandle, &pException, instructionSet, supportEnabled);
if (pException != nullptr) throw pException;
return temp;
}

virtual void allocMem(
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14328,12 +14328,13 @@ void CEEInfo::GetProfilingHandle(bool *pbHookFunction,
UNREACHABLE(); // only called on derived class.
}

void CEEInfo::notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet,
bool CEEInfo::notifyInstructionSetUsage(CORINFO_InstructionSet instructionSet,
bool supportEnabled)
{
LIMITED_METHOD_CONTRACT;
// Do nothing. This api does not provide value in JIT scenarios and
// crossgen does not utilize the api either.
return supportEnabled;
}

#endif // !DACCESS_COMPILE
Expand Down
Loading

0 comments on commit 518eed2

Please sign in to comment.