-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[Interp] Interpreter for ARM32 SOFTP #120688
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
Draft
clamp03
wants to merge
33
commits into
dotnet:main
Choose a base branch
from
clamp03:intrp_arm
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+697
−23
Draft
Changes from 22 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
c2b6454
[CLRINTRP] Port to ARM
clamp03 ad24b45
[CLRINTRP] Make dummy asssembly functions
clamp03 04b980f
Disable Float
clamp03 8cd71fa
[INTRP] Print HelloWorld Done
clamp03 55560c4
[INTRP] HelloWorld Done
clamp03 7df0312
[INTRP] Update X to R for arm register
clamp03 5b3ef8e
[INTRP] Call methods with 2 args and return value
clamp03 02320b0
[INTRP] Implement Args Load and Store Assemblies
clamp03 cb816e6
[INTRP] Add Load_Ref and Store_Ref
clamp03 0cc0032
[INTRP] Return I8
clamp03 a09723d
[INTRP] Update I8 to I4 for ARM32
clamp03 07d975a
[INTRP] Call to Method with LONG args
clamp03 9875d8b
[INTRP] Initial Support for Float and Double
clamp03 4d7d19f
[INTRP] Update for ARM SOFTFP
clamp03 ab7eca6
[INTRP] Update all assemblies for arm softp
clamp03 1cf65b7
[INTRP] Can Pass Arguments using Stack
clamp03 aabd45b
[INTRP] Remove Redundant Codes
clamp03 e6a79a0
[INTRP] Support Struct
clamp03 a5e1bd5
[INTRP] Support RetBuff
clamp03 b3ba6ec
[DO NOT MERGE] For Test
clamp03 30e25e0
[INTRP] Fix release build errors
clamp03 82769b4
Revert "[DO NOT MERGE] For Test"
clamp03 f402453
Workarounds for errors
clamp03 4d48c03
Revert INTERP_STACK_SLOT_SIZE
clamp03 1607eb2
Revert "Revert INTERP_STACK_SLOT_SIZE"
clamp03 8280ee2
Update StackVal for 32 bit target
clamp03 df468a6
Merge remote-tracking branch 'origin/main' into intrp_arm
clamp03 b93b7cd
Fix align for 8 bytes and value type
clamp03 a9c0d6e
8 bytes stack size for WASM
clamp03 705cf2e
Change interpreter stack size to 8 bytes
clamp03 fe42ce9
Update ProcessArgument
clamp03 c69cb0d
Merge remote-tracking branch 'origin/main' into intrp_arm
clamp03 4e3ac8d
Fix InterpreterStub
clamp03 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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 is a bad idea, I think. It will cause all sorts of mayhem. Is there a particular reason why this needs to happen for your PR to work?
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.
Also, if you change this, StackVal in interpexec.h needs to have its 8-byte elements removed, I believe.
Uh oh!
There was an error while loading. Please reload this page.
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 thought 4bytes is good for ARM32 architecture to sync register size and interpreter stack size. (+ and reduce memory a little.) For 8-byte elements, I changed it to use two stacks in some places.
If you think it is better to keep stack slot size to 8 bytes for ARM32 too, I will update it.
Thank you. I missed.
+ Do you have any test set for interpreter implementation? If you have, could you share tests and how to test? Thank you.
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.
InterpreterTester and Interpreter.cs were where we started testing before we were able to run the whole test suite.
I'll leave it to one of the interpreter architects to say whether the stack slot size should stay at 8, I just wanted to let you know that it has wide-ranging consequences.
For what it's worth, the mono interpreter has 8-byte stack slots even on arm32.
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 will check the implementation with InterpreterTester.
Okay. It can make wide-ranging consequences even though I think there are some benefits for ARM32.
I will revert to 8 byte-stack slot.
Thank you.
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.
@kg I found a problem when I change it to 8 byte-stack slot. (Actually, I forgot implementation details during my long holidays. 🥲)
If I change it to 8 byte-stack slot, it seems passing args between compiled methods and interpreter is hard. When it passes two 4-bytes args or one 8-bytes arg, it uses 2 registers in ARM32. So if it is one 8-bytes arg, values in two registers are needed to be loaded from or stored to
one stack slot. However in case of two 4-bytes args, values in two registers are loaded from or stored totwo stack slots. In current implementation, argument passing is handled by Load_* and Store_* routines in assembly code without any type check. However, if stack slot is 8 bytes, I think it needs to do type check for all args (or make routines for all cases.).How do you solve this in mono interpreter? Could you share any idea about this?
Thank you.
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 believe the mono interpreter does transitions using hand-written C helpers in most cases, so the C compiler solves the problem for us. @BrzVlad would probably know better though.
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.
@kg Thank you. I think 4-bytes stack slot for arm32 isn't so bad idea to me. And if I isolate ARM32 implementation from the other arch well, I think it doesn't make wide-ranging consequences in other archs. What do you think?