Remove CommandQueue redirecting usages straight to HWCQ #17219

ayerofieiev-tt · 2025-01-28T07:40:06Z

Ticket

Problem description

Over time CommandQueue stopped playing any particular function.
Today it is simply proxying calls to HWCQ via creating temporary command structures and immediately routing their execution with a big switch statement, calling into hwcq methods.
This adds unreasonable complexity.

After the sync with @tt-asaigal @dmakoviichuk-tt @cfjchu @omilyutin-tt , we agreed to remove this class.

What's changed

Remove CommandQueue class.
Redirect all usage to HWCQ.
HWCommandQueue is renamed to CommandQueue

Note:

command_queue.hpp was not removed. It contains definitions of commands used by hardware_command_queue, I would prefer us to clean this up in the next PR.

Checklist

omilyutin-tt

Pretty straightforward IMO, thanks for tackling this Artem!

Do we want to rename HWCommandQueue to CommandQueue though?

tt_metal/impl/device/device.cpp

omilyutin-tt · 2025-01-28T15:18:03Z

tt_metal/api/tt-metalium/hardware_command_queue.hpp

-    friend void EnqueueTraceImpl(CommandQueue& cq, uint32_t trace_id, bool blocking);
-    friend void EnqueueProgramImpl(CommandQueue& cq, Program& program, bool blocking);
-    friend void EnqueueReadBufferImpl(
-        CommandQueue& cq,
-        std::variant<std::reference_wrapper<Buffer>, std::shared_ptr<Buffer>> buffer,
-        void* dst,
-        const BufferRegion& region,
-        bool blocking);
-    friend void EnqueueWriteBufferImpl(
-        CommandQueue& cq,
-        const std::variant<std::reference_wrapper<Buffer>, std::shared_ptr<Buffer>>& buffer,
-        HostDataType src,
-        const BufferRegion& region,
-        bool blocking);
-    friend void EnqueueGetBufferAddrImpl(void* dst_buf_addr, const Buffer* buffer);
-    friend void EnqueueRecordEventImpl(
-        CommandQueue& cq, const std::shared_ptr<Event>& event, tt::stl::Span<const SubDeviceId> sub_device_ids);
-    friend void EnqueueWaitForEventImpl(CommandQueue& cq, const std::shared_ptr<Event>& event);
-    friend void FinishImpl(CommandQueue& cq, tt::stl::Span<const SubDeviceId> sub_device_ids);
-    friend CommandQueue;
-    friend detail::Program_;


First time I looked at HWCQ I could not understand what it queues if there are no public methods to enqueue. Then realized we are doing a trick.

tt-asaigal

Thanks Artem, this gets the ball rolling on an important change :) Left a few comments.

tt-asaigal · 2025-01-28T16:49:26Z

tt_metal/api/tt-metalium/command_queue.hpp

@@ -228,114 +226,21 @@ struct CommandInterface {
    tt::stl::Span<const SubDeviceId> sub_device_ids;


Can we please remove this while we're at it? This was used for deprecated CommandQueue functionality.
Same for RuntimeArgsMetadata.

tt-asaigal · 2025-01-28T16:52:08Z

tt_metal/api/tt-metalium/command_queue.hpp

-    inline static uint32_t num_passthrough_cqs = 0;
-};
-
-}  // namespace v0


Thanks for removing this class and cleaning things up. We have a bunch of command structs defined in this file, ex: EnqueueTraceCommand, EnqueueProgramCommand and EnqueueTerminateCommand.
These are used inside the individual enqueue impls to do command specific processing. These should not be in this file. Can we please move them?

Agree. Want to make this in the PR after this is merged together with file names.

tt-asaigal · 2025-01-28T17:00:01Z

tt_metal/impl/dispatch/command_queue.cpp

        "Command Queue could not finish: device hang due to illegal NoC transaction. See {} for details.",
        tt::watcher_get_log_file_name());
 }

-void EnqueueTrace(CommandQueue& cq, uint32_t trace_id, bool blocking) {
+void EnqueueTrace(HWCommandQueue& cq, uint32_t trace_id, bool blocking) {
    detail::DispatchStateCheck(true);


All Enqueue* APIs are defined here. Wondering if this is the right way of doing things long term. I think eventually these should be moved out of this file since these are just wrappers to member functions, and don't really have anything to do with the CommandQueue impl itself. Perhaps tt_metal.cpp or some other API specific file would be more suited here.

Yes, I agree. They don't belong here long term.

Not moving them right now to minimize changes in this PR.

ayerofieiev-tt · 2025-01-28T18:17:30Z

tt_metal/api/tt-metalium/command_queue.hpp

@@ -22,20 +22,18 @@
 #include "worker_config_buffer.hpp"
 #include "program_impl.hpp"
 #include "trace_buffer.hpp"
+#include "hardware_command_queue.hpp"


Want to fix file names and includes in a separate PR

cfjchu · 2025-01-28T19:22:35Z

tt_metal/api/tt-metalium/hardware_command_queue.hpp

@@ -104,10 +102,48 @@ class HWCommandQueue {
    void set_expected_num_workers_completed_for_sub_device(uint32_t sub_device_index, uint32_t num_workers);


@tt-asaigal these methods will get cleaned up? These were added mainly for MeshCommandQueue?

ayerofieiev-tt · 2025-01-28T20:03:05Z

tt_metal/impl/device/device.cpp

@@ -998,13 +996,15 @@ void Device::init_command_queue_device() {
 }

 void Device::initialize_synchronous_sw_cmd_queue() {
+    TT_THROW("Slow Dispatch is not supported in this change. Help needed to make it right 😱");


Need to fix, cause this breaks tests

dmakoviichuk-tt · 2025-01-28T21:45:21Z

tt_metal/api/tt-metalium/hardware_command_queue.hpp

 private:
-    uint32_t id;
+    uint32_t id_;


could you add default initialization if you are already here :)

Yes, but not in this PR.
I had to touch these members to comply with what command_queue gave before

dmakoviichuk-tt · 2025-01-28T21:45:57Z

tt_metal/api/tt-metalium/hardware_command_queue.hpp

+        const BufferRegion& region,
+        bool blocking,
+        tt::stl::Span<const SubDeviceId> sub_device_ids = {});
+    void enqueue_read_buffer(


I think it is a time to get rid of this overload.

haha. not time yet. minimizing changes only to the core of it.

dmakoviichuk-tt · 2025-01-28T21:46:12Z

tt_metal/api/tt-metalium/hardware_command_queue.hpp

+    void enqueue_wait_for_event(const std::shared_ptr<Event>& sync_event, bool clear_count = false);
+
+    void enqueue_write_buffer(
+        const std::variant<std::reference_wrapper<Buffer>, std::shared_ptr<Buffer>>& buffer,


tt-dma

fyi @nhuang-tt

tt-aho · 2025-01-29T00:48:42Z

tests/tt_metal/tt_metal/api/test_CommandQueue.cpp

-TEST_F(CommandQueueFixture, TestCannotAccessCommandQueueForClosedDevice) {
-    EXPECT_NO_THROW(device_->command_queue());
-    CloseDevice(device_);
-    EXPECT_ANY_THROW(device_->command_queue());
-}


Do we care about keeping this test?

There is no old CommandQueue anymore

I understand the old CommandQueue is removed.

This test technically works for both the old CommandQueue and the HardwareCommandQueue so I was wondering if it was worth keeping around.

…to query cq from device. cq might not be present in case of slow dispatch

@tt-asaigal

### Ticket #17208 ### Problem description Over time `CommandQueue` stopped playing any particular function. Today it is simply proxying calls to HWCQ via creating temporary command structures and immediately routing their execution with a big switch statement, calling into hwcq methods. This adds unreasonable complexity. After the sync with @tt-asaigal @dmakoviichuk-tt @cfjchu @omilyutin-tt , we agreed to remove this class. ### What's changed Remove CommandQueue class. Redirect all usage to HWCQ. HWCommandQueue is renamed to CommandQueue Note: * command_queue.hpp was not removed. It contains definitions of commands used by hardware_command_queue, I would prefer us to clean this up in the next PR. ### Checklist - [x] [Post commit CI](https://github.com/tenstorrent/tt-metal/actions/runs/13023052350) - [x] [Blackhole Post commit](https://github.com/tenstorrent/tt-metal/actions/runs/13023279261) - [x] [T3k Frequent](https://github.com/tenstorrent/tt-metal/actions/runs/13021799389) -

@tt-asaigal

### Ticket #17208 ### Problem description Over time `CommandQueue` stopped playing any particular function. Today it is simply proxying calls to HWCQ via creating temporary command structures and immediately routing their execution with a big switch statement, calling into hwcq methods. This adds unreasonable complexity. After the sync with @tt-asaigal @dmakoviichuk-tt @cfjchu @omilyutin-tt , we agreed to remove this class. ### What's changed Remove CommandQueue class. Redirect all usage to HWCQ. HWCommandQueue is renamed to CommandQueue Note: * command_queue.hpp was not removed. It contains definitions of commands used by hardware_command_queue, I would prefer us to clean this up in the next PR. ### Checklist - [x] [Post commit CI](https://github.com/tenstorrent/tt-metal/actions/runs/13023052350) - [x] [Blackhole Post commit](https://github.com/tenstorrent/tt-metal/actions/runs/13023279261) - [x] [T3k Frequent](https://github.com/tenstorrent/tt-metal/actions/runs/13021799389) -

@tt-asaigal

…17219) ### Ticket tenstorrent#17208 ### Problem description Over time `CommandQueue` stopped playing any particular function. Today it is simply proxying calls to HWCQ via creating temporary command structures and immediately routing their execution with a big switch statement, calling into hwcq methods. This adds unreasonable complexity. After the sync with @tt-asaigal @dmakoviichuk-tt @cfjchu @omilyutin-tt , we agreed to remove this class. ### What's changed Remove CommandQueue class. Redirect all usage to HWCQ. HWCommandQueue is renamed to CommandQueue Note: * command_queue.hpp was not removed. It contains definitions of commands used by hardware_command_queue, I would prefer us to clean this up in the next PR. ### Checklist - [x] [Post commit CI](https://github.com/tenstorrent/tt-metal/actions/runs/13023052350) - [x] [Blackhole Post commit](https://github.com/tenstorrent/tt-metal/actions/runs/13023279261) - [x] [T3k Frequent](https://github.com/tenstorrent/tt-metal/actions/runs/13021799389) -

ayerofieiev-tt added 3 commits January 28, 2025 06:42

remove friends from HWCQ

7b93564

Remove all usages of CommandQueue

51ed99a

revert addition of lfs and taskflow

a05f98e

ayerofieiev-tt changed the title ~~Remove CommandQueue class redirecting usages straight to HWCQ~~ Remove CommandQueue redirecting usages straight to HWCQ Jan 28, 2025

omilyutin-tt reviewed Jan 28, 2025

View reviewed changes

rename HWCommandQueue to CommandQueue. Files to be renamed in another PR

47dc587

tt-asaigal reviewed Jan 28, 2025

View reviewed changes

ayerofieiev-tt mentioned this pull request Jan 28, 2025

Add Light Metal capture/replay initial changes to tt-metal for some workloads #16573

Open

4 tasks

ayerofieiev-tt commented Jan 28, 2025

View reviewed changes

cfjchu approved these changes Jan 28, 2025

View reviewed changes

ayerofieiev-tt commented Jan 28, 2025

View reviewed changes

dmakoviichuk-tt reviewed Jan 28, 2025

View reviewed changes

remove redundant structures

baffafd

ayerofieiev-tt marked this pull request as ready for review January 28, 2025 23:57

ayerofieiev-tt requested review from TT-BrianLiu, aliuTT, abhullar-tt, pgkeller, tt-aho, tt-dma, ubcheema and davorchap as code owners January 28, 2025 23:57

Merge branch 'main' into ay/hwcq_friends

b6ce1b3

tt-asaigal approved these changes Jan 29, 2025

View reviewed changes

TT-BrianLiu approved these changes Jan 29, 2025

View reviewed changes

tt-dma approved these changes Jan 29, 2025

View reviewed changes

tt-aho approved these changes Jan 29, 2025

View reviewed changes

remove cq id param from APIs that dont use it to not force consumers …

9acc448

…to query cq from device. cq might not be present in case of slow dispatch

ayerofieiev-tt added 2 commits January 29, 2025 02:08

remove query to command_queue from test_dispatcher

52e8f1a

fix docs

e9f0be6

omilyutin-tt approved these changes Jan 29, 2025

View reviewed changes

ayerofieiev-tt changed the title ~~Remove CommandQueue redirecting usages straight to HWCQ~~ 17208: Remove CommandQueue redirecting usages straight to HWCQ Jan 29, 2025

ayerofieiev-tt changed the title ~~17208: Remove CommandQueue redirecting usages straight to HWCQ~~ #17208: Remove CommandQueue redirecting usages straight to HWCQ Jan 29, 2025

ayerofieiev-tt changed the title ~~#17208: Remove CommandQueue redirecting usages straight to HWCQ~~ Remove CommandQueue redirecting usages straight to HWCQ Jan 29, 2025

ayerofieiev-tt linked an issue Jan 29, 2025 that may be closed by this pull request

Replace CommandQueue in MeshDevice #17208

Closed

dmakoviichuk-tt approved these changes Jan 29, 2025

View reviewed changes

Merge branch 'main' into ay/hwcq_friends

c9f5d94

ayerofieiev-tt merged commit 58f9654 into main Jan 29, 2025
10 checks passed

ayerofieiev-tt deleted the ay/hwcq_friends branch January 29, 2025 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove CommandQueue redirecting usages straight to HWCQ #17219

Remove CommandQueue redirecting usages straight to HWCQ #17219

ayerofieiev-tt commented Jan 28, 2025 •

edited

Loading

omilyutin-tt left a comment

omilyutin-tt Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

tt-asaigal left a comment

tt-asaigal Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

tt-asaigal Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

tt-asaigal Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

cfjchu Jan 28, 2025

tt-asaigal Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

dmakoviichuk-tt Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

dmakoviichuk-tt Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

dmakoviichuk-tt Jan 28, 2025

ayerofieiev-tt Jan 28, 2025

tt-dma left a comment

tt-aho Jan 29, 2025

ayerofieiev-tt Jan 29, 2025

tt-aho Jan 29, 2025

		@@ -228,114 +226,21 @@ struct CommandInterface {
		tt::stl::Span<const SubDeviceId> sub_device_ids;

		@@ -104,10 +102,48 @@ class HWCommandQueue {
		void set_expected_num_workers_completed_for_sub_device(uint32_t sub_device_index, uint32_t num_workers);

Remove CommandQueue redirecting usages straight to HWCQ #17219

Remove CommandQueue redirecting usages straight to HWCQ #17219

Conversation

ayerofieiev-tt commented Jan 28, 2025 • edited Loading

Ticket

Problem description

What's changed

Checklist

omilyutin-tt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tt-asaigal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tt-dma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayerofieiev-tt commented Jan 28, 2025 •

edited

Loading