- Notifications
You must be signed in to change notification settings - Fork 5.2k
Jit preparation for arm64 apple: Use bytes in fgArgTabEntry. #42503
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
Conversation
bebb74c to 9422786 Compare fgArgTabEntry. It will fail on arm64 apple.
9422786 to 1aba9e4 Compare 1aba9e4 to 1ca1a42 Compare | PTAL @dotnet/jit-contrib |
CarolEidt left a comment
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.
Overall LGTM, with some mostly style-related suggestions.
src/coreclr/src/jit/jit.h Outdated
| #endif // defined(UNIX_AMD64_ABI) | ||
| | ||
| #if defined(DEBUG) && !defined(OSX_ARM64_ABI) | ||
| #define DEBUG_NOT_OSX_ARM64_ABI |
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.
IMO these would be better left as, e.g. defined(DEBUG) && !defined(OSX_ARM64_ABI) at each use. I don't find the implied conjunction clear enough, though I see that it might be difficult to deal with the #defines below. Perhaps you could define something like TRACK_ARG_SLOTS or DEBUG_ARG_SLOTS and have a comment here about why it is not used for the OSX_ARM64_ABIT case?
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 would say it is a temporary solution, all code under DEBUG_NOT_OSX_ARM64_ABI will go away once the work is done and we check that the new byte mechanism is correct and matches the old results. However, I have found it useful to use a new define because now I can easily change it:
- define it in release for non apple arm64;
- replace
assert-s withnoway_assert-s and make debugging easier;
etc.
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, I agree that it's not a huge deal since it's temporary, but I appreciate you making the changes~
cdf55db to e674e5a Compare
CarolEidt left a comment
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.
LGTM - thanks!
In order to support Arm64 Apple ABI we need to know the exact size of small arguments because when they are passed on stack they are passed without alignment.
There are two main parts that need changes:
fgArgInfoandGenTreePutArgStk. In this PR I am addingbyteSize/byteOffsettofgArgInfo. I am keeping the old `stackSlot/stackSize(in target_pointer sized slots) in place for now but use it only in asserts under debug. I will delete them after all changes are in and we confirm that both models give us the same results on non affected platforms.No diffs(both jit-diff and spmi).
I recommend reviewing by commits (because the first are just refactoring changes):
d888195: fix a dumping error introduced in my recent ref PR.
5f5e631: add a pinvoke test with many small stack arguments.
It will fail on arm64 apple.
345d355: Add get/set for lclVar::lvStkOffs.
20fdade: Change some types from
size_ttounsignedwhen working with stack offsets.80beef0: Rename
stackSizetoGetStackSize.49ed384: Do not run stack level setter for arm64 apple.
I had a prototype that was supporting byte sizes there but the cost was too high for this PR. The main issue is that we will have to track alignment between the currect argument and the next one and currently it is not working well. For example, the stack level setter ignores arm32 double alignment requirements and gives wrong results there when I fixed it I got asm diffs because of the #42673. The stack level setter does not affect arm64 because there we always save stack frame so it is safe to skip it there.
1ca1a42: Use bytes in
fgArgTabEntry.The main change, I could put it in a separate PR if that will make review easier.