Handle more than 64 registers - Part 1#101950
Merged
kunalspathak merged 10 commits intodotnet:mainfrom May 11, 2024
Merged
Conversation
ghost
added
the
area-CodeGen-coreclr
Contributor
Author
|
@dotnet/jit-contrib |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
Author
|
This is expected to be zero TP diff, so I will investigate why it is causing 2% regression. Possibly I missed updating something:
|
Contributor
Author
|
for windows/arm64 crossgen2 collection, here is the distribution. Will take a look |
jakobbotsch
reviewed
May 7, 2024
jakobbotsch
reviewed
May 7, 2024
jakobbotsch
reviewed
May 7, 2024
jakobbotsch
reviewed
May 7, 2024
jakobbotsch
reviewed
May 7, 2024
Contributor
Author
|
The culprit was using |
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
src/coreclr/jit/target.h
Outdated
Comment on lines
241
to
244
| FORCEINLINE explicit operator unsigned int() const | ||
| { | ||
| return (unsigned int)low; | ||
| } |
Member
There was a problem hiding this comment.
This one we ought to remove as well.
Contributor
Author
There was a problem hiding this comment.
It is currently used only at one place. Ideally, all the *refRegs should be just gpr register set of size 4 bytes, but do not want to mix that change in this PR.
if (ig->igFlags & IGF_BYREF_REGS)
{
// Record the byref regs in front the of the instructions
*castto(id, unsigned*)++ = (unsigned)emitInitByrefRegs;
}
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
src/coreclr/jit/compiler.hpp
Outdated
Comment on lines
984
to
988
| #ifdef TARGET_ARM64 | ||
| regNumber regNum = (regNumber)BitScanForward(mask); | ||
| #else | ||
| regNumber regNum = (regNumber)BitOperations::BitScanForward(mask); | ||
| #endif |
Member
There was a problem hiding this comment.
It would be good to avoid these ifdefs by introducing some forwarding BitScanForward in the typedef unsigned __int64 regMaskTP case.
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
Member
Yeah, seems to just be various MSVC regressions due to now having a struct instead of primitive type. Clang seems to do a little bit better. Not much we can do about that I think. |
Member
|
We might consider switching Alternatively we could move extra operations to live on some |
Contributor
Author
I don't mind doing it and was advocate of similar idea back in #98258 because with that, in future, when we add APX support for Intel, enabling the "handling of 64 registers". Edit: With that said, given that #98258 is already out for couple of months now and it is a critical work that is needed to make other progress for SVE, I would like to concentrate on enabling it with as minimal work as needed (just for arm64) and have it enable for other platforms as a follow up PRs once we complete the "predicate register" work. |
jakobbotsch
reviewed
May 10, 2024
jakobbotsch
reviewed
May 10, 2024
src/coreclr/jit/target.h
Outdated
Comment on lines
253
to
256
| FORCEINLINE void setLow(uint64_t _low) | ||
| { | ||
| low = _low; | ||
| } |
Member
There was a problem hiding this comment.
I don't think we should have this one (can just assign the full regMaskTP instead)
Member
There was a problem hiding this comment.
Feel free to include it in the follow up
Member
There was a problem hiding this comment.
(FWIW, it seems like bad encapsulation to have low accessible outside this class at all)
Contributor
Author
There was a problem hiding this comment.
I totally agree with you and was not a fan of having this in the last commit. May be I should just try assigning the whole regMaskTP.
jakobbotsch
approved these changes
May 10, 2024
Contributor
Author
|
/azp run runtime-coreclr superpmi-diffs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
Author
|
|
Contributor
Author
|
/azp run runtime-coreclr superpmi-replay |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Ruihan-Yin
pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
* Convert regMaskTP for ARM64 to struct with single field * Fix genFirstRegNumFromMaskAndToggle() and genFirstRegNumFromMask() * minor fix * review feedback * fix the TP regression from 1.5% -> 0.5% * Pass by value * jit format * review feedback * Remove FORCEINLINE * Remove setLow()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The general feedback for #98258 was to come up with smaller PRs concentrated around LSRA. This is part 1 of that.
For Arm64, this PR changes the
typedef unsigned __int64 regMaskTPto `typedefA version of
PopCountandBitOperationshas been added next toregMaskTPstruct definition.Most of the method implementation is pulled from #96196.