-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsCurrently since
|
c502277
to
54860ef
Compare
CC. @davidwrighton and @dotnet/crossgen-contrib This needs review and sign-off to ensure this doesn't negatively impact the CG2 scenarios. Please call out any other outerloop validation that should be done as well. |
public: | ||
static const int FlagsArrSize = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this up to the beginning of the struct and use it in the definition of _flags
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will move FlagsArrSize
. It is currently used for definition of _flags
but I'm using the actual calculated value instead of the variable since we know at the time of generating this file what the value is. But I don't see a problem with switching to var as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight correction here - the reason FlagsArrSize
is currently public is because it's used from compiler.h. I see that you have another comment there. Will address that first before changing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
src/coreclr/jit/compiler.h
Outdated
@@ -8966,19 +8973,23 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |||
JitFlags* jitFlags; // all flags passed from the EE | |||
|
|||
// The instruction sets that the compiler is allowed to emit. | |||
uint64_t compSupportsISA; | |||
uint64_t compSupportsISA[CORINFO_InstructionSetFlags::FlagsArrSize]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason we cannot use CORINFO_InstructionSetFlags
here and avoid all this manual bit fiddling when setting/getting members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean have this as following and use the CORINFO_InstructionSetFlags methods to do the manipulation?
CORINFO_InstructionSetFlags compSupportsISA;
..........................
CORINFO_InstructionSetFlags compSupportsISAReported;
........................
CORINFO_InstructionSetFlags compSupportsISAExactly;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Changed
private static int _flagsArrSize = 1; | ||
private fixed ulong _flags[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this can use a const
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
54860ef
to
db63099
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any specific suggestions in addition to @jakobbotsch .
I'd like to see if we can reduce the number of uses of the "Raw" APIs, but that's for a follow-up.
_flags = _flags | (((uint64_t)1) << instructionSet); | ||
int arrayIdx = instructionSet / 64; | ||
int bit = instructionSet % 64; | ||
_flags[arrayIdx] = _flags[arrayIdx] | (((uint64_t)1) << bit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use |=
?
_flags[arrayIdx] = _flags[arrayIdx] | (((uint64_t)1) << bit); | |
_flags[arrayIdx] |= ((uint64_t)1) << bit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
_flags = _flags & ~(((uint64_t)1) << instructionSet); | ||
int arrayIdx = instructionSet / 64; | ||
int bit = instructionSet % 64; | ||
_flags[arrayIdx] = _flags[arrayIdx] & ~(((uint64_t)1) << bit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_flags[arrayIdx] = _flags[arrayIdx] & ~(((uint64_t)1) << bit); | |
_flags[arrayIdx] &= ~(((uint64_t)1) << bit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
db63099
to
21f93aa
Compare
21f93aa
to
47a064e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. It'd like @davidwrighton to take a look at the AOT side.
BTW, is it reasonable to convert this from a "Draft" to a "normal" PR? Normally, I think of "Draft" as something that is not ready to be reviewed. |
Yes, this is ready to be reviewed and I've taken out the 'draft' tag. Will keep this in mind for newer PRs(i.e., tagging as draft initially to see test run results and if it broke something unexpected and then removing 'draft' tag once it looks good) |
public void AddInstructionSet(InstructionSet instructionSet) | ||
{ | ||
_flags = _flags | (((ulong)1) << (int)instructionSet); | ||
int arrayIdx = (int)instructionSet / 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to use uint in this pattern. Signed division is less efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Good catch- it's a power of 2 division. Changed
uint64_t* GetFlagsRaw() | ||
{ | ||
return _flags; | ||
} | ||
|
||
void SetFromFlagsRaw(uint64_t flags) | ||
void SetFromFlagsRaw(uint64_t* flags) | ||
{ | ||
_flags = flags; | ||
for (int i = 0; i < FlagsArrSize; i++) | ||
{ | ||
_flags[i] = flags[i]; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want/need to still allow getting the raw flags? I think we could just return and enforce usage of CORINFO_InstructionSetFlags
instead.
It looks like the "raw" APIs are really only used from class CORJIT_FLAGS
and setSupportedISAs
which normally abstracts away the underlying CORINFO_InstructionSetFlags
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that it makes sense to remove the SetFromFlagsRaw
but GetFlagsRaw
is used from SuperPMI(see changes in src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
). I might be missing something here but I am not sure how I can work around that.
src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetGenerator.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetGenerator.cs
Outdated
Show resolved
Hide resolved
cbf2c55
to
043aae7
Compare
@DeepakRajendrakumaran Can you fix the JIT formatting? (See the "Formatting" job failures) Also, there are some SuperPMI asm diffs: I would expect this change to not have any asm diffs. Agreed? |
There is a small change in the SuperPMI tool but it ideally shouldn't have any effect as far as I understand. Will check it out. |
Ah, you've changed CORINFO_InstructionSetFlags and thus CORJIT_FLAGS, which is passed across the JIT interface. You should change the JIT-EE GUID in jiteeversionguid.h. I thought your change wouldn't change data layout so maybe it should "just work" with a SPMI collection with the previous layout, but maybe not? |
|
||
static uint64_t GetRelativeBitIndex(CORINFO_InstructionSet instructionSet) | ||
{ | ||
return ((uint64_t)1) << (instructionSet & 0x3F); |
There was a problem hiding this comment.
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
return ((uint64_t)1) << (instructionSet & 0x3F); | |
uint32_t bitIndex = (uint32_t)instructionSet; | |
return (uint32_t)(bitIndex % (uint32_t)BitsPerFlagsField); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
||
private static ulong GetRelativeBitIndex(InstructionSet instructionSet) | ||
{ | ||
return ((ulong)1) << ((int)instructionSet & 0x3F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ((ulong)1) << ((int)instructionSet & 0x3F); | |
uint bitIndex = (uint)instructionSet; | |
return (uint)(bitIndex % (uint)BitsPerFlagsField); |
|
||
private static ulong GetRelativeBitIndex(InstructionSet instructionSet) | ||
{ | ||
return ((ulong)1) << ((int)instructionSet & 0x3F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ((ulong)1) << ((int)instructionSet & 0x3F); | |
uint bitIndex = (uint)instructionSet; | |
return (uint)(bitIndex % (uint)BitsPerFlagsField); |
|
||
static uint64_t GetRelativeBitIndex(CORINFO_InstructionSet instructionSet) | ||
{ | ||
return ((uint64_t)1) << (instructionSet & 0x3F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ((uint64_t)1) << (instructionSet & 0x3F); | |
uint32_t bitIndex = (uint32_t)instructionSet; | |
return (uint32_t)(bitIndex % (uint32_t)BitsPerFlagsField); |
Another question is, right now I'm just copying the struct( Refer src/coreclr/jit/jitee.h for example |
043aae7
to
6cb3350
Compare
Yeah. I didn't excpect any changes. I followed the instruction here(link) and ran the following
I take the last line |
True. I wonder why the Note that now that you've changed the JIT-EE GUID, you won't be able to run SuperPMI diffs.
Normally, I would recommend using a Checked build (just because it's faster). Also, if you're on a Windows x64 machine, the -target_os / -target_arch / -arch will be defaulted as you've specified. |
Is this something you have seen before with JITInterface changes?(specifically this -
This makes it difficult to check the original failures. Is there some way to see the original SuperPMI failures we need to look at/fix? Or do I have to revert this temporarily and get the CI to re-run everything? |
It turns out we realized late last week that our automatic JIT baseline build system wasn't working properly. That should now be fixed. If you rebase on top of current main (and re-push), the system should pick up a current baseline.
It might make sense to temporarily revert the JIT-EE version GUID change if you want to re-run testing. Note this will only work if there is no binary difference in the JIT-EE traffic (including the CORFLAGS stuff), which I believe should be the case with your change. |
b0fa75d
to
5386480
Compare
Yep. It worked. All tests passed. I'll remove the 'revert' commit and change |
I didn't read the full thread -- is the JIT-EE GUID change still necessary now that we established the diffs were spurious? |
It's a fair question. The data representation changed, but at least currently in a binary identical way. When the number of flags is increased, we'll certainly need to change the GUID. So for now, it's ok not to change the GUID (and not changing it leads to less churn). |
In the GUID file it says
Also from updating-jitinterface.md
There has been some changes in the interface code. But as you pointed out, no diff changes exist. So, I'll leave it to the reviewers to decide. :) Edit : Didn't see the reply above when I posted this comment. Will follow the guidance there. |
5386480
to
efc70f7
Compare
Details: Previously ISA flags were represented as bits in a 64 bit variable. This limited the total number of possible ISAs to 64. This change modifies this and starts using an array of 64 bit vars to store ISA flags. The main changes are in InstructionSetGenerator.cs. This drives a lot of other changes in this commit which are generated files.
@dotnet/crossgen-contrib @davidwrighton @trylek -- Anyone want to take a look at this JIT-EE interface format change for potential crossgen2 impact? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As no jitinterface changes are part of this PR, I believe our only interaction with the JIT flags is in JitConfigProvider and we seem to be consistently using CorJitFlag arrays to represent them; I believe this change should have zero Crossgen2 impact.
@trylek, are there any concerns with how this will impact crossgen each time we do need to increase the field count? We are definitely going to need at least 2 fields for the .NET 8 AVX-512 work, so ensuring that this schema will work and work over time without too much additional effort is also important. Ideally simply changing the |
@tannergooding - going over the code a second time I have found one place where we still manipulate JIT flags using the
In Crossgen2 its only users seem to be two places that use the To my knowledge today Crossgen2 doesn't support command-line parameters for specifying arbitrary JIT flags, it only uses about a dozen flags that are hard-coded in the source code (a bunch of flags affecting optimizations, instruction sets and the like). From this viewpoint future extensions of Crossgen2 support for JIT flags may be twofold:
|
@trylek Thanks for the explanation!
In particular, I was considering the metadata used to track the instruction sets that a given method uses (required vs opportunistic). I believe this is related to the
Extending the number of |
I believe that today implementation of instruction sets uses ReadyToRun fixups, the exact encoding is implemented here: Line 90 in 8641256
As you can easily see, it doesn't have the 64-bit limitation as it encodes the individual instruction sets as signature-encoded uints. You are right that we'd need to revamp the R2R format if we wanted to support multitargeting compilation (where a function could have several implementations compiled for various instruction sets and a suitable version would be picked dynamically at runtime) but that would be a completely new feature so it would require design in any case. |
Thanks for confirming. Just wanted to ensure that us going over wasn't going to cause unexpected breakages in R2R due to some missed edge case. @BruceForstall, I think this is ready to merge then once your "change requested" is resolved. |
Currently since
_flags
is a 64 bit variable, only 64 ISAs can be represented using this design. This change makes this an array thus enabling us to represent more than 64 ISAs.