LightMetal - Add LoadTrace() API and move TraceDescriptor out of detail namespace (#17039) #17313
Conversation
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
kmabeeTT
requested review from
cfjchu,
aliuTT,
tt-asaigal,
omilyutin-tt,
abhullar-tt,
pgkeller,
tt-aho,
tt-dma,
ubcheema and
davorchap
as code owners
This was referenced Jan 29, 2025
omilyutin-tt
requested changes
Jan 29, 2025
| @@ -312,7 +312,7 @@ EnqueueTraceCommand::EnqueueTraceCommand( | |||
| uint32_t command_queue_id, | |||
| IDevice* device, | |||
| SystemMemoryManager& manager, | |||
| std::shared_ptr<detail::TraceDescriptor>& descriptor, | |||
| std::shared_ptr<TraceDescriptor>& descriptor, | |||
There was a problem hiding this comment.
Yikes, I think all of these can and should be const &. (outside of the PR, just a drive by comment:) )
| this->id_, | ||
| active_sub_device_manager->id()); | ||
|
|
||
| auto& trace_buffer = active_sub_device_manager->create_trace(trace_id); |
There was a problem hiding this comment.
(outside of this PR)
These are weird APIs that return references to smart pointers. @dmakoviichuk-tt wdyt? I think these:
const std::unique_ptr<Allocator>& get_initialized_allocator(SubDeviceId sub_device_id) const;
std::unique_ptr<Allocator>& sub_device_allocator(SubDeviceId sub_device_id);
std::shared_ptr<TraceBuffer>& create_trace(uint32_t tid);
Should be re-written as:
const Allocator* get_initialized_allocator(SubDeviceId sub_device_id) const;
Allocator* sub_device_allocator(SubDeviceId sub_device_id);
std::shared_ptr<TraceBuffer> create_trace(uint32_t tid);
…il namespace (#17039) - Will be used by Light Metal replay after upcoming PR, when executing a metal-trace traced program from binary, TraceDescriptor is extracted from flatbuffer binary and loaded to device through this API. - Unrelated - Change trace_buffer.hpp to use fwd decl Buffer instead of buffer.hpp incl to reduce dependencies on users of trace_buffer.hpp
kmabeeTT
force-pushed
the
kmabee/light_metal_add_apis2
branch
from
6294297 to
d1eb7be
Compare
omilyutin-tt
approved these changes
Jan 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ticket
I am breaking up original PR I put up recently into smaller bite-sized chunks as @omilyutin-tt suggested. Here is round 3.
Problem description
This is initial/bootstrapping changes for "Light Metal" capture/replay feature that uses Flatbufers as serialization/deserialization library. See my previous bigger PR (#16573) if you want to see how this will be used by followup merge, or context.
What's changed
LoadTrace(IDevice* device, const uint8_t cq_id, const uint32_t trace_id, TraceDescriptor& trace_desc)that is unused now, but will be used by Light Metal replay after upcoming PR, when executing a metal-trace traced program from binary, TraceDescriptor is extracted from flatbuffer binary and loaded to device through this API.TraceDescriptorout of detail namespace after discussion in parent PR. Don't want to expose detail objects on public APIs. Thetrace_buffer.hppthat containsTraceDescriptorwas already in public API folder (nice).I incorporated some feedback in parent PR to use "trace_id" as variable name instead of "tid" (which is departure from other trace-related APIs, but in order to keep this PR contained, not updating existing APIs).
Checklist