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

Allow more than 64 ISA flags for jit interface. #73965

Merged
merged 1 commit into from
Aug 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
71 changes: 57 additions & 14 deletions src/coreclr/inc/corinfoinstructionset.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,41 +128,89 @@ enum CORINFO_InstructionSet
struct CORINFO_InstructionSetFlags
{
private:
uint64_t _flags = 0;
static const int32_t FlagsFieldCount = 1;
static const int32_t BitsPerFlagsField = sizeof(uint64_t) * 8;
uint64_t _flags[FlagsFieldCount] = { };


static uint32_t GetFlagsFieldIndex(CORINFO_InstructionSet instructionSet)
{
uint32_t bitIndex = (uint32_t)instructionSet;
return (uint32_t)(bitIndex / (uint32_t)BitsPerFlagsField);
}

static uint64_t GetRelativeBitMask(CORINFO_InstructionSet instructionSet)
{
return ((uint64_t)1) << (instructionSet & 0x3F);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a bug, since it ignores the top 2 bits

Suggested change
return ((uint64_t)1) << (instructionSet & 0x3F);
uint32_t bitIndex = (uint32_t)instructionSet;
return (uint32_t)(bitIndex % (uint32_t)BitsPerFlagsField);

Copy link
Member

Choose a reason for hiding this comment

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

GetFlagsFieldIndex gets the 64-bit field in which a given instructionSet is tagged (only the bits 6 and higher matter).

GetRelativeBitIndex then gets the bit offset within that field, which can only be 0-63, so 0x3F should be the correct mask.

Copy link
Member

Choose a reason for hiding this comment

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

GetRelativeBitMask might have been a better name, since its not getting the index but rather a mask that only includes the given index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have changed name to GetRelativeBitMask

}

public:

const int GetInstructionFlagsFieldCount() const
{
return FlagsFieldCount;
}

void AddInstructionSet(CORINFO_InstructionSet instructionSet)
{
_flags = _flags | (((uint64_t)1) << instructionSet);
uint32_t index = GetFlagsFieldIndex(instructionSet);
_flags[index] |= GetRelativeBitMask(instructionSet);
}

void RemoveInstructionSet(CORINFO_InstructionSet instructionSet)
{
_flags = _flags & ~(((uint64_t)1) << instructionSet);
uint32_t index = GetFlagsFieldIndex(instructionSet);
uint64_t bitIndex = GetRelativeBitMask(instructionSet);
_flags[index] &= ~bitIndex;
}

bool HasInstructionSet(CORINFO_InstructionSet instructionSet) const
{
return _flags & (((uint64_t)1) << instructionSet);
uint32_t index = GetFlagsFieldIndex(instructionSet);
uint64_t bitIndex = GetRelativeBitMask(instructionSet);
return ((_flags[index] & bitIndex) != 0);
}

bool Equals(CORINFO_InstructionSetFlags other) const
{
return _flags == other._flags;
for (int i = 0; i < FlagsFieldCount; i++)
{
if (_flags[i] != other._flags[i])
{
return false;
}

}
return true;
}

void Add(CORINFO_InstructionSetFlags other)
{
_flags |= other._flags;
for (int i = 0; i < FlagsFieldCount; i++)
{
_flags[i] |= other._flags[i];
}
}

bool IsEmpty() const
{
return _flags == 0;
for (int i = 0; i < FlagsFieldCount; i++)
{
if (_flags[i] != 0)
{
return false;
}

}
return true;
}

void Reset()
{
_flags = 0;
for (int i = 0; i < FlagsFieldCount; i++)
{
_flags[i] = 0;
}
}

void Set64BitInstructionSetVariants()
Expand Down Expand Up @@ -230,15 +278,10 @@ struct CORINFO_InstructionSetFlags

}

uint64_t GetFlagsRaw()
uint64_t* GetFlagsRaw()
{
return _flags;
}

void SetFromFlagsRaw(uint64_t flags)
{
_flags = flags;
}
};

inline CORINFO_InstructionSetFlags EnsureInstructionSetFlagsAreValid(CORINFO_InstructionSetFlags input)
Expand Down
12 changes: 11 additions & 1 deletion src/coreclr/inc/corjitflags.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,21 @@ class CORJIT_FLAGS
}

// DO NOT USE THIS FUNCTION! (except in very restricted special cases)
uint64_t GetInstructionSetFlagsRaw()
uint64_t* GetInstructionSetFlagsRaw()
{
return instructionSetFlags.GetFlagsRaw();
}

CORINFO_InstructionSetFlags GetInstructionSetFlags()
{
return instructionSetFlags;
}

const int GetInstructionFlagsFieldCount()
{
return instructionSetFlags.GetInstructionFlagsFieldCount();
}

private:

uint64_t corJitFlags;
Expand Down
7 changes: 3 additions & 4 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2270,10 +2270,9 @@ void Compiler::compSetProcessor()
// the total sum of flags is still valid.

CORINFO_InstructionSetFlags instructionSetFlags = jitFlags.GetInstructionSetFlags();

opts.compSupportsISA = 0;
opts.compSupportsISAReported = 0;
opts.compSupportsISAExactly = 0;
opts.compSupportsISA.Reset();
opts.compSupportsISAReported.Reset();
opts.compSupportsISAExactly.Reset();

#if defined(TARGET_XARCH)
instructionSetFlags.AddInstructionSet(InstructionSet_Vector128);
Expand Down
25 changes: 12 additions & 13 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -8835,7 +8835,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
bool compIsaSupportedDebugOnly(CORINFO_InstructionSet isa) const
{
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
return (opts.compSupportsISA & (1ULL << isa)) != 0;
return opts.compSupportsISA.HasInstructionSet(isa);
#else
return false;
#endif
Expand All @@ -8850,14 +8850,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
bool compExactlyDependsOn(CORINFO_InstructionSet isa) const
{
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
uint64_t isaBit = (1ULL << isa);
if ((opts.compSupportsISAReported & isaBit) == 0)
if ((opts.compSupportsISAReported.HasInstructionSet(isa)) == false)
{
if (notifyInstructionSetUsage(isa, (opts.compSupportsISA & isaBit) != 0))
((Compiler*)this)->opts.compSupportsISAExactly |= isaBit;
((Compiler*)this)->opts.compSupportsISAReported |= isaBit;
if (notifyInstructionSetUsage(isa, (opts.compSupportsISA.HasInstructionSet(isa))))
((Compiler*)this)->opts.compSupportsISAExactly.AddInstructionSet(isa);
((Compiler*)this)->opts.compSupportsISAReported.AddInstructionSet(isa);
}
return (opts.compSupportsISAExactly & isaBit) != 0;
return (opts.compSupportsISAExactly.HasInstructionSet(isa));
#else
return false;
#endif
Expand All @@ -8879,7 +8878,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
// If the result is false, then the target machine may have support for the instruction.
bool compOpportunisticallyDependsOn(CORINFO_InstructionSet isa) const
{
if ((opts.compSupportsISA & (1ULL << isa)) != 0)
if (opts.compSupportsISA.HasInstructionSet(isa))
{
return compExactlyDependsOn(isa);
}
Expand All @@ -8894,7 +8893,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
{
// Report intent to use the ISA to the EE
compExactlyDependsOn(isa);
return ((opts.compSupportsISA & (1ULL << isa)) != 0);
return opts.compSupportsISA.HasInstructionSet(isa);
}

bool canUseVexEncoding() const
Expand Down Expand Up @@ -8999,19 +8998,19 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
JitFlags* jitFlags; // all flags passed from the EE

// The instruction sets that the compiler is allowed to emit.
uint64_t compSupportsISA;
CORINFO_InstructionSetFlags compSupportsISA;
// The instruction sets that were reported to the VM as being used by the current method. Subset of
// compSupportsISA.
uint64_t compSupportsISAReported;
CORINFO_InstructionSetFlags 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;
CORINFO_InstructionSetFlags compSupportsISAExactly;

void setSupportedISAs(CORINFO_InstructionSetFlags isas)
{
compSupportsISA = isas.GetFlagsRaw();
compSupportsISA = isas;
}

unsigned compFlags; // method attributes
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/jitee.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ class JitFlags
{
// We don't want to have to check every one, so we assume it is exactly the same values as the JitFlag
// values defined in this type.
m_jitFlags = flags.GetFlagsRaw();
m_instructionSetFlags.SetFromFlagsRaw(flags.GetInstructionSetFlagsRaw());
m_jitFlags = flags.GetFlagsRaw();
m_instructionSetFlags = flags.GetInstructionSetFlags();

C_ASSERT(sizeof(JitFlags) == sizeof(CORJIT_FLAGS));

Expand Down
65 changes: 53 additions & 12 deletions src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ public enum InstructionSet
X86_MOVBE_X64 = InstructionSet_X86.MOVBE_X64,
X86_X86Serialize_X64 = InstructionSet_X86.X86Serialize_X64,
}

public enum InstructionSet_ARM64
{
ILLEGAL = InstructionSet.ILLEGAL,
Expand Down Expand Up @@ -240,54 +239,96 @@ public enum InstructionSet_X86
X86Serialize_X64 = 40,
}

public struct InstructionSetFlags : IEnumerable<InstructionSet>
public unsafe struct InstructionSetFlags : IEnumerable<InstructionSet>
{
private ulong _flags;

private const int FlagsFieldCount = 1;
private const int BitsPerFlagsField = 64;
private fixed ulong _flags[FlagsFieldCount];
public IEnumerable<InstructionSet_ARM64> ARM64Flags => this.Select((x) => (InstructionSet_ARM64)x);

public IEnumerable<InstructionSet_X64> X64Flags => this.Select((x) => (InstructionSet_X64)x);

public IEnumerable<InstructionSet_X86> X86Flags => this.Select((x) => (InstructionSet_X86)x);

public InstructionSetFlags() { }

private static uint GetFlagsFieldIndex(InstructionSet instructionSet)
{
uint bitIndex = (uint)instructionSet;
return (uint)(bitIndex / (uint)BitsPerFlagsField);
}

private static ulong GetRelativeBitMask(InstructionSet instructionSet)
{
return ((ulong)1) << ((int)instructionSet & 0x3F);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return ((ulong)1) << ((int)instructionSet & 0x3F);
uint bitIndex = (uint)instructionSet;
return (uint)(bitIndex % (uint)BitsPerFlagsField);

}

public void AddInstructionSet(InstructionSet instructionSet)
{
_flags = _flags | (((ulong)1) << (int)instructionSet);
uint index = GetFlagsFieldIndex(instructionSet);
_flags[index] |= GetRelativeBitMask(instructionSet);
}

public void RemoveInstructionSet(InstructionSet instructionSet)
{
_flags = _flags & ~(((ulong)1) << (int)instructionSet);
uint index = GetFlagsFieldIndex(instructionSet);
ulong bitIndex = GetRelativeBitMask(instructionSet);
_flags[index] &= ~bitIndex;
}

public bool HasInstructionSet(InstructionSet instructionSet)
{
return (_flags & (((ulong)1) << (int)instructionSet)) != 0;
uint index = GetFlagsFieldIndex(instructionSet);
ulong bitIndex = GetRelativeBitMask(instructionSet);
return ((_flags[index] & bitIndex) != 0);
}

public bool Equals(InstructionSetFlags other)
{
return _flags == other._flags;
for (int i = 0; i < FlagsFieldCount; i++)
{
if (_flags[i] != other._flags[i])
{
return false;
}
}
return true;
}

public void Add(InstructionSetFlags other)
{
_flags |= other._flags;
for (int i = 0; i < FlagsFieldCount; i++)
{
_flags[i] |= other._flags[i];
}
}

public void IntersectionWith(InstructionSetFlags other)
{
_flags &= other._flags;
for (int i = 0; i < FlagsFieldCount; i++)
{
_flags[i] &= other._flags[i];
}
}

public void Remove(InstructionSetFlags other)
{
_flags &= ~other._flags;
for (int i = 0; i < FlagsFieldCount; i++)
{
_flags[i] &= ~other._flags[i];
}
}

public bool IsEmpty()
{
return _flags == 0;
for (int i = 0; i < FlagsFieldCount; i++)
{
if (_flags[i] != 0)
{
return false;
}
}
return true;
}

IEnumerator IEnumerable.GetEnumerator()
Expand Down
Loading