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.
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
Unroll Buffer.Memmove for constant lengths #83638
Unroll Buffer.Memmove for constant lengths #83638
Changes from all commits
9f45dbf
cc66929
60cf0ea
2ec2e4c
33d9c4e
42e325f
adc1da3
b6a11f1
a3b90e6
2bb7dfe
c569201
9ea8e86
ff66578
aa3e32d
e228f97
f4a9174
2124513
0fbc82c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just curious -- how do we expect the logic/heuristics to be different on x86?
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.
special byte-register for 1 byte access, up to 4 instead of 2 GPR for 1..15 range. I plan to file a follow up to add arm64 and then I'll probably add x86 and arm32 when I have time
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.
Why would we use more registers on x86, there are less registers available?
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.
No, but in order to memmove 15 bytes you need
4*4b
registers to load it while on x64 you need2*8b
.So I only implemented x64 to simplify code review for the initial version. Once it's merged it'd be eaiser to review a PR that adds x86
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.
Unused?
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 used in
LowerCallMemmove
. Currently it's not handled ingetThreshould
inside and treated just like memcpy, but it allows future changes for memcpy only since memmove has a hard limit to grow.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 not obvious what the significance of this comment is at a glance.
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's just a hint to whoever decides to change the thresholds. E.g. I planned to play with the thresholds for memset/memcpy for very hot places based on PGO data. But me (or whoever) will have to be careful with memmove
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.
Couldn't this return
call->gtNext
and then we could avoid all the special-case checking for nullptr?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 can't return
call->gtNext
here becuasegtNext
can be nullptr itself so then it won't be clear whether this method made any changes or not. In fact, I did it initally and hit an assert somewhere