-
Notifications
You must be signed in to change notification settings - Fork 31
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
Delay stack size allocation until after core code generated #1104
Draft
newling
wants to merge
9
commits into
nod-ai:main
Choose a base branch
from
newling:jit_stack
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.
Draft
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9382355
first commit
newling dc1aeb2
first commit
newling e79acff
test updates
newling f94c8d1
gate chess
newling bc9a193
last attempt
newling 4d44d2a
rm
newling 96a6a3c
squish
newling 7acf46b
tidy
newling e1cd157
generalize padd
newling 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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.
Is this assuming that the stack is located at address 0 and use offsets relative to that? If so, I don't think we should make that assumption. To avoid bank conflicts, I will soon want to be able to assign buffers strategically across the memory banks at specific addresses and an address relative to the stack makes that impossible.
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.
So previously if we had
stackSize = 1024, address = 1040
Now we havestack_relative_address = 16
and the stack size is set later. We can set stack size to be any value larger than what the final code actually needs (largest offset from stack pointer in object dump).So we stack size to 1024, the program is completely unchanged from before this PR. The stack is at address 0, I don't see any reason to change that. If you mean you'd like some additional memory left as a gap between the stack and the first buffer, that is easy to insert.
These are the lines where the addresses of buffers are set:
https://github.com/nod-ai/iree-amd-aie/pull/1104/files#diff-5a1e140ca056a489d6cf9766431fb5529180ec410b726425dc8a4208f8af477eL57
So before
address
was assigned incrementally starting atstackSize
. Now,stack_relative_address
is assigned incrementally starting at 0.Can you provide more information of what you have in mind? Also, do you see this PR as moving further from your goal than what we currently have?
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.
The problem with assigning buffers relative to the stack address is that we won't be able to control the exact locations of other buffers, for example:
stack size is 4096, buffer A0 (ping) has size 8192, buffer A1 (pong) has size 8192, buffer B0 has size 8192 etc
Addresses assigned will be:
Now, this will result in two DMAs operating on the same bank 2 at some points + the vector processor as well and this will result in bank conflicts.
What I would like to achieve is something like:
This assumes the stack is located somewhere else, but even if you would assume the stack is located at address 0 and we would like to use banks 2/3 for the above buffers, that can't be achieved with an address relative to the stack:
I know this is the case as well with the current assignment as the stack is located at address 0 and buffers are appended after that and we will need better assignment strategies to avoid that. But we can't avoid these bank conflicts without granular control of where to locate buffers.
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.
Got it. I think placing the stack somewhere other than at address 0 will be a hard task, it will be easier (for now) to start assigning buffers from the end of the memory (64K?). And the we start having banks too. Let me play around with this
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.
Ideally, we would place all buffers after we know all sizes, but that doesn't seem easy right now as the buffer addresses are provided as an input to peano. Alternatively, we can put an upper bound on the stack size and error out if that would be exceeded, which would be an improvement already. Taking that further, we could place the stack buffer at the largest unused data memory block available and error out if that was not enough. This would work as well with more intelligent buffer placement strategies.