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.

Sign up for GitHub

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

Jump to bottom

Unify instruction set definition #33730

Merged
merged 6 commits into from
Mar 19, 2020
Merged

Unify instruction set definition #33730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6952c06
Unify instruction set definition
davidwrighton Mar 18, 2020
4f14632
Fix formatting and build issues
davidwrighton Mar 18, 2020
be186f9
Respond to code review feedback
davidwrighton Mar 18, 2020
26a9810
Fix diffs between old model and new model
davidwrighton Mar 19, 2020
6a65d97
Improve readability of InstructionSetDesc.txt, and fix vm side X86 bu…
davidwrighton Mar 19, 2020
c799dc9
Add 64 bit variant names to 32bit instruction set enum to remove the …
davidwrighton Mar 19, 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
53 changes: 51 additions & 2 deletions src/coreclr/src/inc/corinfoinstructionset.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,64 @@ struct CORINFO_InstructionSetFlags
_flags = _flags & ~(((uint64_t)1) << instructionSet);
}

bool HasInstructionSet(CORINFO_InstructionSet instructionSet)
bool HasInstructionSet(CORINFO_InstructionSet instructionSet) const
{
return _flags & (((uint64_t)1) << instructionSet);
}
bool Equals(CORINFO_InstructionSetFlags other)

bool Equals(CORINFO_InstructionSetFlags other) const
{
return _flags == other._flags;
}

void Add(CORINFO_InstructionSetFlags other)
{
_flags |= other._flags;
}

bool IsEmpty() const
{
return _flags == 0;
}

void Reset()
{
_flags = 0;
}

void Set64BitInstructionSetVariants()
{
#ifdef TARGET_ARM64
if (HasInstructionSet(InstructionSet_ArmBase))
AddInstructionSet(InstructionSet_ArmBase_Arm64);
if (HasInstructionSet(InstructionSet_AdvSimd))
AddInstructionSet(InstructionSet_AdvSimd_Arm64);
if (HasInstructionSet(InstructionSet_Crc32))
AddInstructionSet(InstructionSet_Crc32_Arm64);
#endif // TARGET_ARM64
#ifdef TARGET_AMD64
if (HasInstructionSet(InstructionSet_SSE))
AddInstructionSet(InstructionSet_SSE_X64);
if (HasInstructionSet(InstructionSet_SSE2))
AddInstructionSet(InstructionSet_SSE2_X64);
if (HasInstructionSet(InstructionSet_SSE41))
AddInstructionSet(InstructionSet_SSE41_X64);
if (HasInstructionSet(InstructionSet_SSE42))
AddInstructionSet(InstructionSet_SSE42_X64);
if (HasInstructionSet(InstructionSet_BMI1))
AddInstructionSet(InstructionSet_BMI1_X64);
if (HasInstructionSet(InstructionSet_BMI2))
AddInstructionSet(InstructionSet_BMI2_X64);
if (HasInstructionSet(InstructionSet_LZCNT))
AddInstructionSet(InstructionSet_LZCNT_X64);
if (HasInstructionSet(InstructionSet_POPCNT))
AddInstructionSet(InstructionSet_POPCNT_X64);
#endif // TARGET_AMD64
#ifdef TARGET_X86
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming, this is meant to be empty and isn't a bug?

Copy link
Member

Choose a reason for hiding this comment

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

If so, having the generator skip if empty or leave a comment if empty might be more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

#endif // TARGET_X86

}

uint64_t GetFlagsRaw()
{
return _flags;
Expand Down
14 changes: 8 additions & 6 deletions src/coreclr/src/inc/corjitflags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ class CORJIT_FLAGS
void Reset()
{
corJitFlags = 0;
instructionSetFlags.Reset();
}

void Set(CORINFO_InstructionSet instructionSet)
Expand All @@ -155,6 +156,11 @@ class CORJIT_FLAGS
instructionSetFlags.RemoveInstructionSet(instructionSet);
}

void Set64BitInstructionSetVariants()
{
instructionSetFlags.Set64BitInstructionSetVariants();
}

void Set(CorJitFlag flag)
{
corJitFlags |= 1ULL << (unsigned __int64)flag;
Expand All @@ -173,16 +179,12 @@ class CORJIT_FLAGS
void Add(const CORJIT_FLAGS& other)
{
corJitFlags |= other.corJitFlags;
}

void Remove(const CORJIT_FLAGS& other)
{
corJitFlags &= ~other.corJitFlags;
instructionSetFlags.Add(other.instructionSetFlags);
}

bool IsEmpty() const
{
return corJitFlags == 0;
return corJitFlags == 0 && instructionSetFlags.IsEmpty();
}

void EnsureValidInstructionSetSupport()
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/src/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5285,9 +5285,8 @@ int Compiler::compCompile(CORINFO_METHOD_HANDLE methodHnd,

CORINFO_InstructionSetFlags defaultArm64Flags;
defaultArm64Flags.AddInstructionSet(InstructionSet_ArmBase);
defaultArm64Flags.AddInstructionSet(InstructionSet_ArmBase_Arm64);
defaultArm64Flags.AddInstructionSet(InstructionSet_AdvSimd);
defaultArm64Flags.AddInstructionSet(InstructionSet_AdvSimd_Arm64);
defaultArm64Flags.Set64BitInstructionSetVariants();
compileFlags->SetInstructionSetFlags(defaultArm64Flags);
#endif // defined(TARGET_ARM64)
}
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/pal/src/misc/jitsupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,5 @@ PAL_GetJitCpuCapabilityFlags(CORJIT_FLAGS *flags)
// CPUCompileFlags.Set(CORJIT_FLAGS::CORJIT_FLAG_HAS_ARM64_FP);
#endif // HAVE_AUXV_HWCAP_H
#endif // defined(HOST_ARM64)
CPUCompileFlags.Set64BitInstructionSetVariants();
}
8 changes: 2 additions & 6 deletions src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2883,9 +2883,7 @@ private uint getJitFlags(ref CORJIT_FLAGS flags, uint sizeInBytes)
else if (targetArchitecture == TargetArchitecture.X64)
Copy link
Member

Choose a reason for hiding this comment

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

We could combine the x86 and x64 codepaths and just add the *_X64 variants for x64.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no guarantee that the X86 and X64 variants actually use the same constants. (It so happens they do at the moment, but that is one of the few actually negative consequences of this change, which is that the constants could become out of sync.)

Copy link
Member

Choose a reason for hiding this comment

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

That is unfortunate, as it means we have a lot of duplicated code between the two of them (and that will likely grow over time).

Copy link
Contributor

Choose a reason for hiding this comment

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

That does seem unfortunate. Does it really add a lot of complexity to share them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it would be quite a bit of complexity, as the generator is pretty dumb. Mind you, the followon work is expected to delete this (the only logic which adds this duplication of effort) in favor of some string handling which will make all of this unified again. I hope to put together that PR by end of week, assuming I can get this in tonight or tomorrow morning, so I don't see much value in investing in a bit of code that should only live for about 48-96 hours before beginning active replacement.

{
flags.Set(InstructionSet.X64_SSE);
flags.Set(InstructionSet.X64_SSE_X64);
flags.Set(InstructionSet.X64_SSE2);
flags.Set(InstructionSet.X64_SSE2_X64);
#if !READYTORUN
// This list needs to match the list of intrinsics we can generate detection code for
// in HardwareIntrinsicHelpers.EmitIsSupportedIL.
Expand All @@ -2901,14 +2899,10 @@ private uint getJitFlags(ref CORJIT_FLAGS flags, uint sizeInBytes)
flags.Set(InstructionSet.X64_SSE3);
flags.Set(InstructionSet.X64_SSSE3);
flags.Set(InstructionSet.X64_LZCNT);
flags.Set(InstructionSet.X64_LZCNT_X64);
#if READYTORUN
flags.Set(InstructionSet.X64_SSE41);
flags.Set(InstructionSet.X64_SSE41_X64);
flags.Set(InstructionSet.X64_SSE42);
flags.Set(InstructionSet.X64_SSE42_X64);
flags.Set(InstructionSet.X64_POPCNT);
flags.Set(InstructionSet.X64_POPCNT_X64);
#endif
}
}
Expand All @@ -2918,6 +2912,8 @@ private uint getJitFlags(ref CORJIT_FLAGS flags, uint sizeInBytes)
flags.Set(InstructionSet.ARM64_AdvSimd);
}

flags.Set64BitInstructionSetVariants(targetArchitecture);

if (this.MethodBeingCompiled.IsNativeCallable)
{
#if READYTORUN
Expand Down
39 changes: 39 additions & 0 deletions src/coreclr/src/tools/Common/JitInterface/CorInfoInstructionSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,5 +266,44 @@ public static InstructionSetFlags ExpandInstructionSetByImplication(TargetArchit

}
}

public void Set64BitInstructionSetVariants(TargetArchitecture architecture)
{
switch (architecture)
{

case TargetArchitecture.ARM64:
if (HasInstructionSet(InstructionSet.ARM64_ArmBase))
AddInstructionSet(InstructionSet.ARM64_ArmBase_Arm64);
if (HasInstructionSet(InstructionSet.ARM64_AdvSimd))
AddInstructionSet(InstructionSet.ARM64_AdvSimd_Arm64);
if (HasInstructionSet(InstructionSet.ARM64_Crc32))
AddInstructionSet(InstructionSet.ARM64_Crc32_Arm64);
break;

case TargetArchitecture.X64:
if (HasInstructionSet(InstructionSet.X64_SSE))
AddInstructionSet(InstructionSet.X64_SSE_X64);
if (HasInstructionSet(InstructionSet.X64_SSE2))
AddInstructionSet(InstructionSet.X64_SSE2_X64);
if (HasInstructionSet(InstructionSet.X64_SSE41))
AddInstructionSet(InstructionSet.X64_SSE41_X64);
if (HasInstructionSet(InstructionSet.X64_SSE42))
AddInstructionSet(InstructionSet.X64_SSE42_X64);
if (HasInstructionSet(InstructionSet.X64_BMI1))
AddInstructionSet(InstructionSet.X64_BMI1_X64);
if (HasInstructionSet(InstructionSet.X64_BMI2))
AddInstructionSet(InstructionSet.X64_BMI2_X64);
if (HasInstructionSet(InstructionSet.X64_LZCNT))
AddInstructionSet(InstructionSet.X64_LZCNT_X64);
if (HasInstructionSet(InstructionSet.X64_POPCNT))
AddInstructionSet(InstructionSet.X64_POPCNT_X64);
break;

case TargetArchitecture.X86:
break;

}
}
}
}
15 changes: 3 additions & 12 deletions src/coreclr/src/tools/Common/JitInterface/CorInfoTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Diagnostics;
using System.Runtime.InteropServices;
using Internal.TypeSystem;

namespace Internal.JitInterface
{
Expand Down Expand Up @@ -1354,19 +1355,9 @@ public bool IsSet(CorJitFlag flag)
return (_corJitFlags & (1UL << (int)flag)) != 0;
}

public void Add(ref CORJIT_FLAGS other)
public void Set64BitInstructionSetVariants(TargetArchitecture architecture)
{
_corJitFlags |= other._corJitFlags;
}

public void Remove(ref CORJIT_FLAGS other)
{
_corJitFlags &= ~other._corJitFlags;
}

public bool IsEmpty()
{
return _corJitFlags == 0;
_instructionSetFlags.Set64BitInstructionSetVariants(architecture);
}
}
}
69 changes: 67 additions & 2 deletions src/coreclr/src/tools/Common/JitInterface/ThunkGenerator/InstructionSetGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,33 @@ public static InstructionSetFlags ExpandInstructionSetByImplication(TargetArchit
tr.Write(@"
}
}

public void Set64BitInstructionSetVariants(TargetArchitecture architecture)
{
switch (architecture)
{
");
foreach (string architecture in _architectures)
{
tr.Write($@"
case TargetArchitecture.{architecture}:
");
foreach (var instructionSet in _instructionSets)
{
if (instructionSet.Architecture != architecture) continue;

if (_64bitVariants[architecture].Contains(instructionSet.JitName))
{
tr.WriteLine($" if (HasInstructionSet(InstructionSet.{architecture}_{instructionSet.JitName}))");
tr.WriteLine($" AddInstructionSet(InstructionSet.{architecture}_{instructionSet.JitName}_{ArchToInstructionSetSuffixArch(architecture)});");
}
}

tr.WriteLine(" break;");
}
tr.Write(@"
}
}
}
}
");
Expand Down Expand Up @@ -457,15 +484,53 @@ void RemoveInstructionSet(CORINFO_InstructionSet instructionSet)
_flags = _flags & ~(((uint64_t)1) << instructionSet);
}

bool HasInstructionSet(CORINFO_InstructionSet instructionSet)
bool HasInstructionSet(CORINFO_InstructionSet instructionSet) const
{
return _flags & (((uint64_t)1) << instructionSet);
}
bool Equals(CORINFO_InstructionSetFlags other)

bool Equals(CORINFO_InstructionSetFlags other) const
{
return _flags == other._flags;
}

void Add(CORINFO_InstructionSetFlags other)
{
_flags |= other._flags;
}

bool IsEmpty() const
{
return _flags == 0;
}

void Reset()
{
_flags = 0;
}

void Set64BitInstructionSetVariants()
{
");
foreach (string architecture in _architectures)
{
tr.WriteLine($"#ifdef TARGET_{ArchToIfDefArch(architecture)}");
foreach (var instructionSet in _instructionSets)
{
if (instructionSet.Architecture != architecture) continue;

if (_64bitVariants[architecture].Contains(instructionSet.JitName))
{
tr.WriteLine($" if (HasInstructionSet(InstructionSet_{instructionSet.JitName}))");
tr.WriteLine($" AddInstructionSet(InstructionSet_{instructionSet.JitName}_{ArchToInstructionSetSuffixArch(architecture)});");
}
}

tr.WriteLine($"#endif // TARGET_{ArchToIfDefArch(architecture)}");
}
tr.Write(@"
}

uint64_t GetFlagsRaw()
{
return _flags;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/vm/codeman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,8 @@ void EEJitManager::SetCpuInfo()
#endif // HOST_64BIT
#endif // TARGET_ARM64

CPUCompileFlags.Set64BitInstructionSetVariants();

m_CPUCompileFlags = CPUCompileFlags;
}

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/zap/zapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,7 @@ void Zapper::InitializeCompilerFlags(CORCOMPILE_VERSION_INFO * pVersionInfo)
#endif // defined(TARGET_X86) || defined(TARGET_AMD64)
}
#endif // defined(TARGET_X86) || defined(TARGET_AMD64) || defined(TARGET_ARM64)
m_pOpt->m_compilerFlags.Set64BitInstructionSetVariants();

if ( m_pOpt->m_compilerFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_INFO)
&& m_pOpt->m_compilerFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE)
Expand Down