-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Jit preparation for arm64 apple: Use bytes in fgArgTabEntry
.
#42503
Conversation
bebb74c
to
9422786
Compare
fgArgTabEntry
.
It will fail on arm64 apple.
9422786
to
1aba9e4
Compare
1aba9e4
to
1ca1a42
Compare
PTAL @dotnet/jit-contrib |
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
@@ -245,6 +245,20 @@ | |||
#define UNIX_AMD64_ABI_ONLY(x) | |||
#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
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:
fgArgInfo
andGenTreePutArgStk
. In this PR I am addingbyteSize/byteOffset
tofgArgInfo
. 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_t
tounsigned
when working with stack offsets.80beef0: Rename
stackSize
toGetStackSize
.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.