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

#10860: Split Conv2dConfig into Compute config #15164

Merged
merged 23 commits into from
Dec 9, 2024
Merged

#10860: Split Conv2dConfig into Compute config #15164

merged 23 commits into from
Dec 9, 2024

Conversation

sankarmanoj-tt
Copy link
Contributor

@sankarmanoj-tt sankarmanoj-tt commented Nov 18, 2024 •
edited
Loading

Ticket

#10860

Problem description

Conv2dConfig contains Compute kernel specific arguments which should be passed using DeviceComputeKernelConfig

What's changed

Removed compute kernel arguments from Conv2dConfig. conv2d takes an additional argument called compute_config. Used pybind to expose helper functions to create DeviceComputeKernelConfig from the model code.

Checklist

@sankarmanoj-tt sankarmanoj-tt marked this pull request as ready for review November 19, 2024 04:44
@sankarmanoj-tt sankarmanoj-tt force-pushed the smanoj/conv_compute_config branch from 91b8981 to c74f9c3 Compare November 19, 2024 09:30
pavlejosipovic
pavlejosipovic reviewed Nov 19, 2024
View reviewed changes
ttnn/ttnn/device.py Outdated Show resolved Hide resolved
pavlejosipovic
pavlejosipovic reviewed Nov 19, 2024
View reviewed changes
ttnn/cpp/ttnn/operations/conv/conv_transpose2d/conv_transpose2d.cpp Outdated
@@ -102,8 +102,10 @@ Result conv_transpose2d(
std::array<uint32_t, 2> dilation,
uint32_t groups,
std::optional<const ttnn::Tensor> bias_tensor,
std::optional<const conv2d::Conv2dConfig> conv_config_) {
std::optional<const conv2d::Conv2dConfig> conv_config_,
std::optional<const DeviceComputeKernelConfig> compute_config_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do pass by const ref for compute_config_, not just here but in general

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should conv_config also be pass by const ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make this change in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think there is PR in flight that does this in general
#15187

pavlejosipovic
pavlejosipovic reviewed Nov 19, 2024
View reviewed changes
Copy link
Contributor

@pavlejosipovic pavlejosipovic left a comment

Choose a reason for hiding this comment

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

You need go over all instances of ttnn.Conv2dConfig() in the code and fixup params.
I see that many of them still accept math_fidelity for example

@sankarmanoj-tt sankarmanoj-tt force-pushed the smanoj/conv_compute_config branch from 0728049 to 290ed51 Compare November 19, 2024 19:51
mywoodstock
mywoodstock reviewed Nov 20, 2024
View reviewed changes
ttnn/cpp/ttnn/operations/conv/conv_transpose2d/conv_transpose2d.cpp Outdated
}
if (conv_config.reallocate_halo_output) {
auto move_output = ttnn::operations::core::reallocate(halo_output, halo_output.memory_config());
ttnn::operations::core::deallocate(halo_output);
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't reallocate already deallocate its input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not certain. From a quick read through of the reallocate implementation, it looks like it does. I have removed the deallocate.

ttnn/ttnn/device.py Outdated
math_approx_mode=math_approx_mode,
dst_full_sync_en=dst_full_sync_en,
)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check if is_grayskull instead of defaulting to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@sankarmanoj-tt sankarmanoj-tt force-pushed the smanoj/conv_compute_config branch from fe4219d to 372d237 Compare November 20, 2024 14:54
@bbradelTT
Copy link
Contributor

Please after this PR look at init_device_compute_kernel_config and see how to remove it.

I assume we just have to construct/build the DeviceKernelConfig. Having method like "init_device.." in ttnn is very misleading. It almost looks like I must call it once to initialize something and thats it.

It's a useful utility function that hides creating an object based on device type. If anything we should rename it to something like get_compute_kernel_config

@sankarmanoj-tt
#0: First commit
06e46c6
@sankarmanoj-tt
#0: Update Models
2912833
@sankarmanoj-tt
#0: Fix Models
a7c0f7e
@sankarmanoj-tt
#0: Revert Compute config defaults
561a37a
@sankarmanoj-tt
#0: Port SD
935c7ad
@sankarmanoj-tt
#0: Seperate Config for Blackhole
b895fc9
@sankarmanoj-tt
#0: Conv2dConfig refactor
bcd7cb7
@sankarmanoj-tt
#0: Shard Layout fix
4c9a0e7
@sankarmanoj-tt
#0: Cleanup
4b9bdb2
@sankarmanoj-tt
#0: Updated Conv2dConfig in test_small_resnet_block
9e5abbd
@sankarmanoj-tt
#0: Set default device for Mesh Device
8a3e5f0
@sankarmanoj-tt
#0: Changed defaults for ttnn.GetComputeKernelConfig
a46b269
@sankarmanoj-tt
#0: Disabled failing SD on Wormhole 7x8
26c5793
@sankarmanoj-tt
#0: Renamed GetComputeKernelConfig to CreateComputeKernelConfig
158ae7e
@sankarmanoj-tt
#0: Alt approach for mesh device
050c987
@sankarmanoj-tt
#0: Removed CreateComputeKernelConfig
281bbc3
@sankarmanoj-tt
#0: Fix device arch
c4dde67
@sankarmanoj-tt
#0: Fixed Stable Diffusion
4d01ddc
@sankarmanoj-tt
#0: Cleanup
aabaee1
@sankarmanoj-tt
#0: Rebase fix
14ea0fa
@sankarmanoj-tt
#0: Cleanup
1e7b91d
@sankarmanoj-tt
#0: Fix matmul in conv_transpose
d08fee1
@sankarmanoj-tt sankarmanoj-tt force-pushed the smanoj/conv_compute_config branch from dad837c to d08fee1 Compare December 9, 2024 05:23
@sankarmanoj-tt sankarmanoj-tt merged commit 612744d into main Dec 9, 2024
119 checks passed
@sankarmanoj-tt sankarmanoj-tt deleted the smanoj/conv_compute_config branch December 9, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tt-aho tt-aho tt-aho left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@ayerofieiev-tt ayerofieiev-tt ayerofieiev-tt approved these changes

@bbradelTT bbradelTT bbradelTT approved these changes

@sjameelTT sjameelTT sjameelTT approved these changes

@esmalTT esmalTT esmalTT approved these changes

@shwetankTT shwetankTT shwetankTT approved these changes

@uaydonat uaydonat uaydonat approved these changes

@tt-rkim tt-rkim tt-rkim approved these changes

@pavlejosipovic pavlejosipovic pavlejosipovic approved these changes

@mywoodstock mywoodstock mywoodstock approved these changes

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

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

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

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

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

@jdesousa-TT jdesousa-TT Awaiting requested review from jdesousa-TT jdesousa-TT is a code owner

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

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

@TT-BrianLiu TT-BrianLiu Awaiting requested review from TT-BrianLiu TT-BrianLiu is a code owner

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

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

@dongjin-na dongjin-na Awaiting requested review from dongjin-na dongjin-na is a code owner

@SeanNijjar SeanNijjar Awaiting requested review from SeanNijjar

@abhullar-tt abhullar-tt Awaiting requested review from abhullar-tt

@pgkeller pgkeller Awaiting requested review from pgkeller

@aliuTT aliuTT Awaiting requested review from aliuTT

@tt-dma tt-dma Awaiting requested review from tt-dma

@tt-asaigal tt-asaigal Awaiting requested review from tt-asaigal

@ubcheema ubcheema Awaiting requested review from ubcheema

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.