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.

Sign up for GitHub

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

Jump to bottom

#14634: Remove usage of ARCH_NAME sp constants MEM_L1_SIZE #14878

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

#14634: Remove usage of ARCH_NAME sp constants MEM_L1_SIZE #14878

wants to merge 7 commits into from

Conversation

blozano-tt
Copy link
Contributor

@blozano-tt blozano-tt commented Nov 8, 2024 •
edited
Loading

Ticket

Closes #14634

Problem description

MEM_L1_BASE, MEM_L1_SIZE
MEM_ETH_BASE, MEM_ETH_SIZE
All come from ARCH_NAME specific includes.

Can't use them if we want a singular build.

What's changed

BASE addresses are always 0x0 independent of architecture, use contexpr.
SIZE can be inferred from HAL, use inline function to compute it.

Checklist

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

  • 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

tt-aho
tt-aho reviewed Nov 9, 2024
View reviewed changes
Copy link
Contributor

@tt-aho tt-aho left a comment

Choose a reason for hiding this comment

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

I'm not really involved with HAL discussion/design, but why do we not want the mem size to be behind the hal?

@pgkeller @abhullar-tt

@blozano-tt blozano-tt added this to the ARCH_NAME Removal milestone Nov 12, 2024
pgkeller
pgkeller requested changes Nov 14, 2024
View reviewed changes
tt_metal/impl/debug/sanitize_noc_host.hpp Outdated Show resolved Hide resolved
tt_metal/impl/debug/sanitize_noc_host.hpp Outdated Show resolved Hide resolved
tt_metal/impl/dispatch/command_queue_interface.hpp Outdated Show resolved Hide resolved
blozano-tt
blozano-tt commented Nov 15, 2024
View reviewed changes
tt_metal/llrt/hal.hpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhullar-tt abhullar-tt Awaiting requested review from abhullar-tt abhullar-tt is a code owner

@aliuTT aliuTT Awaiting requested review from aliuTT aliuTT is a code owner

@tt-dma tt-dma Awaiting requested review from tt-dma tt-dma is a code owner

@tt-asaigal tt-asaigal Awaiting requested review from tt-asaigal tt-asaigal is a code owner

@ubcheema ubcheema Awaiting requested review from ubcheema ubcheema is a code owner

@davorchap davorchap Awaiting requested review from davorchap davorchap is a code owner

@cfjchu cfjchu Awaiting requested review from cfjchu cfjchu is a code owner

@pgkeller pgkeller Awaiting requested review from pgkeller pgkeller is a code owner

@tt-aho tt-aho Awaiting requested review from tt-aho tt-aho is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
arch_name cleanup
Projects
None yet
Milestone
ARCH_NAME Removal
Development

Successfully merging this pull request may close these issues.

command_queue_interface.hpp uses ARCH_NAME specific include path to access MEM_L1_SIZE, MEM_ETH_SIZE
3 participants
@blozano-tt @tt-aho @pgkeller