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

Replace CommandQueue in MeshDevice #17208

Closed
ayerofieiev-tt opened this issue Jan 28, 2025 · 0 comments · Fixed by #17219
Closed

Replace CommandQueue in MeshDevice #17208

ayerofieiev-tt opened this issue Jan 28, 2025 · 0 comments · Fixed by #17219
Assignees

Comments

@ayerofieiev-tt
Copy link
Member

No description provided.

@ayerofieiev-tt ayerofieiev-tt linked a pull request Jan 29, 2025 that will close this issue
3 tasks
ayerofieiev-tt added a commit that referenced this issue Jan 29, 2025
### 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-rkim pushed a commit that referenced this issue Jan 30, 2025
 (#17364)

### Ticket
FYI @tt-rkim 
Related to @ayerofieiev-tt ticket
#17208

Fixes clang-tidy

### Problem description
clang-tidy on CI
([link](https://github.com/tenstorrent/tt-metal/actions/runs/13039390588/job/36414797756))
failing wtih this error since commit
58f9654

```
/home/ubuntu/actions-runner-2/_work/tt-metal/tt-metal/tt_metal/impl/dispatch/command_queue.cpp:469:34: error: the parameter 'runtime_args_ptr' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
  469 |     std::shared_ptr<RuntimeArgs> runtime_args_ptr,
      |                                  ^
      |     const                       &
200249 warnings generated.
Suppressed 200310 warnings (200248 in non-user code, 62 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
```

### What's changed

- Make this argument const ref as suggested and kicked off code-analysis
job on this branch here:
https://github.com/tenstorrent/tt-metal/actions/runs/13053370584

### Checklist
- [ ] Code Analyis job previously failing
- [ ] Post commit CI passes
- [ ] Blackhole Post commit (if applicable)
- [ ] Model regression CI testing passes (if applicable)
- [ ] Device performance regression CI testing passes (if applicable)
- [ ] **(For models and ops writers)** Full [new
models](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
tests passes
- [ ] New/Existing tests provide coverage for changes
williamlyTT pushed a commit that referenced this issue Jan 30, 2025
### 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)
-
williamlyTT pushed a commit that referenced this issue Jan 30, 2025
 (#17364)

### Ticket
FYI @tt-rkim 
Related to @ayerofieiev-tt ticket
#17208

Fixes clang-tidy

### Problem description
clang-tidy on CI
([link](https://github.com/tenstorrent/tt-metal/actions/runs/13039390588/job/36414797756))
failing wtih this error since commit
58f9654

```
/home/ubuntu/actions-runner-2/_work/tt-metal/tt-metal/tt_metal/impl/dispatch/command_queue.cpp:469:34: error: the parameter 'runtime_args_ptr' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
  469 |     std::shared_ptr<RuntimeArgs> runtime_args_ptr,
      |                                  ^
      |     const                       &
200249 warnings generated.
Suppressed 200310 warnings (200248 in non-user code, 62 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
```

### What's changed

- Make this argument const ref as suggested and kicked off code-analysis
job on this branch here:
https://github.com/tenstorrent/tt-metal/actions/runs/13053370584

### Checklist
- [ ] Code Analyis job previously failing
- [ ] Post commit CI passes
- [ ] Blackhole Post commit (if applicable)
- [ ] Model regression CI testing passes (if applicable)
- [ ] Device performance regression CI testing passes (if applicable)
- [ ] **(For models and ops writers)** Full [new
models](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
tests passes
- [ ] New/Existing tests provide coverage for changes
yieldthought pushed a commit that referenced this issue Jan 31, 2025
### 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)
-
yieldthought pushed a commit that referenced this issue Jan 31, 2025
 (#17364)

### Ticket
FYI @tt-rkim 
Related to @ayerofieiev-tt ticket
#17208

Fixes clang-tidy

### Problem description
clang-tidy on CI
([link](https://github.com/tenstorrent/tt-metal/actions/runs/13039390588/job/36414797756))
failing wtih this error since commit
58f9654

```
/home/ubuntu/actions-runner-2/_work/tt-metal/tt-metal/tt_metal/impl/dispatch/command_queue.cpp:469:34: error: the parameter 'runtime_args_ptr' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
  469 |     std::shared_ptr<RuntimeArgs> runtime_args_ptr,
      |                                  ^
      |     const                       &
200249 warnings generated.
Suppressed 200310 warnings (200248 in non-user code, 62 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
```

### What's changed

- Make this argument const ref as suggested and kicked off code-analysis
job on this branch here:
https://github.com/tenstorrent/tt-metal/actions/runs/13053370584

### Checklist
- [ ] Code Analyis job previously failing
- [ ] Post commit CI passes
- [ ] Blackhole Post commit (if applicable)
- [ ] Model regression CI testing passes (if applicable)
- [ ] Device performance regression CI testing passes (if applicable)
- [ ] **(For models and ops writers)** Full [new
models](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
tests passes
- [ ] New/Existing tests provide coverage for changes
nikileshx pushed a commit to nikileshx/tt-metal that referenced this issue Feb 3, 2025
…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)
-
nikileshx pushed a commit to nikileshx/tt-metal that referenced this issue Feb 3, 2025
 (tenstorrent#17364)

### Ticket
FYI @tt-rkim 
Related to @ayerofieiev-tt ticket
tenstorrent#17208

Fixes clang-tidy

### Problem description
clang-tidy on CI
([link](https://github.com/tenstorrent/tt-metal/actions/runs/13039390588/job/36414797756))
failing wtih this error since commit
tenstorrent@58f9654

```
/home/ubuntu/actions-runner-2/_work/tt-metal/tt-metal/tt_metal/impl/dispatch/command_queue.cpp:469:34: error: the parameter 'runtime_args_ptr' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
  469 |     std::shared_ptr<RuntimeArgs> runtime_args_ptr,
      |                                  ^
      |     const                       &
200249 warnings generated.
Suppressed 200310 warnings (200248 in non-user code, 62 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
```

### What's changed

- Make this argument const ref as suggested and kicked off code-analysis
job on this branch here:
https://github.com/tenstorrent/tt-metal/actions/runs/13053370584

### Checklist
- [ ] Code Analyis job previously failing
- [ ] Post commit CI passes
- [ ] Blackhole Post commit (if applicable)
- [ ] Model regression CI testing passes (if applicable)
- [ ] Device performance regression CI testing passes (if applicable)
- [ ] **(For models and ops writers)** Full [new
models](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
tests passes
- [ ] New/Existing tests provide coverage for changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants