Skip to content
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

#11830: Remove more constants from common_runtime_address_map #13785

Closed
wants to merge 6 commits into from

Conversation

blozano-tt
Copy link
Contributor

@blozano-tt blozano-tt commented Oct 14, 2024

Ticket

#11830

Problem description

See issue

What's changed

MEM_MAP_END moved behind Hal
L1_KERNEL_CONFIG_BASE/SIZE is already behind the HAL, remove it from common_runtime_address_map

Checklist

https://github.com/tenstorrent/tt-metal/actions/runs/11337042262

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@blozano-tt blozano-tt marked this pull request as ready for review October 14, 2024 23:03
@@ -41,7 +41,8 @@ enum class HalL1MemAddrType : uint8_t {
CORE_INFO = 7,
GO_MSG = 8,
LAUNCH_MSG_BUFFER_RD_PTR = 9,
COUNT = 10
MEMORY_MAP_END = 10,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with moving L1_KERNEL_CONFIG is that it shouldn't belong in the HAL because it is a host side concept.

HalL1MemAddrType::UNRESERVED should point to MEM_MAP_END (we could rename UNRESERVED to MEMORY_MAP_END) to be more clear.

The original idea was to create a L1 Buffer (that is allocated bottom up) to hold the kernel config and users of HalL1MemAddrType::KERNEL_CONFIG would query device to get the kernel config buffer address. This won't work for ethernet cores because our allocator does not allocate buffers on ethernet cores.

To handle all programmable core types in the same manner, Device could reserve space for kernel config on all programmable core types and then update l1_unreserved_base that is passed into AllocatorConfig to follow kernel config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with moving L1_KERNEL_CONFIG is that it shouldn't belong in the HAL because it is a host side concept.

Can we file a separate issue to track this as an enhancement? Today it is behind Hal, and this PR just leaves it there.

HalL1MemAddrType::UNRESERVED should point to MEM_MAP_END

Currently UNRESERVED is being set differently depending on core type.
I don't see it being set to the equivalent of MEM_MAP_END anywhere, for instance I see this:
((L1_KERNEL_CONFIG_BASE + L1_KERNEL_CONFIG_SIZE - 1) | (max_alignment - 1)) + 1;

Maybe I don't have the necessary background to make this change.

github-actions[bot]

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants