#10671: API to get chip location #10674
Conversation
tt_metal/llrt/tt_cluster.cpp
Outdated
| @@ -341,6 +341,12 @@ uint32_t Cluster::get_harvested_rows(chip_id_t chip) const { | |||
| } | |||
| } | |||
|
|
|||
| eth_coord_t Cluster::get_chip_location(chip_id_t chip) const { | |||
| static std::unordered_map<chip_id_t, eth_coord_t> chip_locations = this->cluster_desc_->get_chip_locations(); | |||
There was a problem hiding this comment.
How expensive is the call get_chip_locations? At first glance making this a static seems ok, but we've had nontrivial problems in the past i.e. the same program/test swaps out a cluster file in the same execution, but the static holds stale state.
There was a problem hiding this comment.
I'm also thinking that typically the call to get_chip_location is only something an app (like ttrt) will do offline and/or infrequently so not too worried about overhead.
There was a problem hiding this comment.
jnie-TT
force-pushed
the
jnie/chip_location_api
branch
from
bf63394 to
87bcf36
Compare
tt_metal/llrt/tt_cluster.cpp
Outdated
| @@ -341,6 +341,12 @@ uint32_t Cluster::get_harvested_rows(chip_id_t chip) const { | |||
| } | |||
| } | |||
|
|
|||
| eth_coord_t Cluster::get_chip_location(chip_id_t chip) const { | |||
| std::unordered_map<chip_id_t, eth_coord_t> chip_locations = this->cluster_desc_->get_chip_locations(); | |||
There was a problem hiding this comment.
Can you make it a reference? Or just return the coord directly this->cluster_desc_->get_chip_locations().at(chip). No need to make a copy here
There was a problem hiding this comment.
Assigned it as reference. Still want to catch the case when the chip isn't in the map instead of debugging map_base::at errors.
jnie-TT
force-pushed
the
jnie/chip_location_api
branch
from
87bcf36 to
71cb95d
Compare
jnie-TT
force-pushed
the
jnie/chip_location_api
branch
from
71cb95d to
323dfa2
Compare
jnie-TT
force-pushed
the
jnie/chip_location_api
branch
from
323dfa2 to
65f9fac
Compare
jnie-TT
force-pushed
the
jnie/chip_location_api
branch
from
65f9fac to
9b98198
Compare
|
@jnie-TT and @jnie-TT: this API is a layer violation, it's a backdoor, and bypasses ttnn and Metal-Runtime APIs. if you require a new API , please file an API request:
Metalium stack has these layers:
Layers shouldn't be bypass, rather if a new API is needed it can be exposed and/or propagated up the layers. |
|
In addition to filing a request, the process for fulling a request is:
|
|
@davorchap I talked to @aliuTT and we agreed to add it as a cluster API and not directly expose it in upper layers. Apologies if this caused any inconvenience, would it be desirable for me to revert this and file an API request? @nsmithtt FYI |
|
Please revert for now. This commit shouldn't be addressing any needs from MLIR, since we should never be bypassing Device apis anyway. Apologies Jackson, I jumped the gun on approving this PR. We should sync first on what APIs to expose even at the ttCluster level that isn't user facing. |
|
Note that there are no CODEOWNERS for this section of the runtime because runtime team previously complained that PRs took too long. I'm going to talk to the runtime team about putting more files under purview, but this also means more PRs will need approval. |
Ticket
Link to Github Issue
What's changed
API added to Device and Cluster to get chip location