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

#15483: Initial setup for binary sfpu ops #15557

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

#15483: Initial setup for binary sfpu ops #15557

wants to merge 2 commits into from

Conversation

KalaivaniMCW
Copy link
Contributor

Ticket

Link to Github Issue #15483

Problem description

Initial setup to test the binary sfpu ops

What's changed

Added program factory and compute kernel for binary sfpu operation
Added test file to check fp32 precision
Overloaded pow tensor-tensor support for testing

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@KalaivaniMCW KalaivaniMCW mentioned this pull request Nov 28, 2024
Open
3 tasks
github-actions[bot]
github-actions bot reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

ttnn/cpp/ttnn/operations/eltwise/binary/common/binary_op_utils.hpp
BinaryOpType op_type,
const std::optional<tt::tt_metal::DataType> in_dtype = std::nullopt,
const std::optional<tt::tt_metal::DataType> out_dtype = std::nullopt,
const std::optional<ttnn::operations::unary::FusedActivations> fused_activations = std::nullopt,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the const qualified parameter fused_activations is copied for each invocation; consider making it a reference

Suggested change
const std::optional<ttnn::operations::unary::FusedActivations> fused_activations = std::nullopt,
const std::optional<ttnn::operations::unary::FusedActivations>& fused_activations = std::nullopt,

ttnn/cpp/ttnn/operations/eltwise/binary/common/binary_op_utils.cpp
BinaryOpType op_type,
const std::optional<tt::tt_metal::DataType> input_dtype,
const std::optional<tt::tt_metal::DataType> output_dtype,
const std::optional<std::vector<UnaryWithParam>> fused_activations,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the const qualified parameter fused_activations is copied for each invocation; consider making it a reference

Suggested change
const std::optional<std::vector<UnaryWithParam>> fused_activations,
const std::optional<std::vector<UnaryWithParam>>& fused_activations,

ttnn/cpp/ttnn/operations/eltwise/binary/common/binary_op_utils.hpp
const std::optional<tt::tt_metal::DataType> in_dtype = std::nullopt,
const std::optional<tt::tt_metal::DataType> out_dtype = std::nullopt,
const std::optional<ttnn::operations::unary::FusedActivations> fused_activations = std::nullopt,
const std::optional<ttnn::operations::unary::UnaryWithParam> input_tensor_a_activation = std::nullopt);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the const qualified parameter input_tensor_a_activation is copied for each invocation; consider making it a reference

Suggested change
const std::optional<ttnn::operations::unary::UnaryWithParam> input_tensor_a_activation = std::nullopt);
const std::optional<ttnn::operations::unary::UnaryWithParam>& input_tensor_a_activation = std::nullopt);

ttnn/cpp/ttnn/operations/eltwise/binary/common/binary_op_utils.cpp
const std::optional<tt::tt_metal::DataType> input_dtype,
const std::optional<tt::tt_metal::DataType> output_dtype,
const std::optional<std::vector<UnaryWithParam>> fused_activations,
const std::optional<unary::UnaryWithParam> input_tensor_a_activation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
the const qualified parameter input_tensor_a_activation is copied for each invocation; consider making it a reference

Suggested change
const std::optional<unary::UnaryWithParam> input_tensor_a_activation) {
const std::optional<unary::UnaryWithParam>& input_tensor_a_activation) {

ttnn/cpp/ttnn/operations/eltwise/unary/device/unary_composite_op.cpp
float exponent,
const std::optional<MemoryConfig>& output_mem_config,
std::optional<Tensor> output_tensor) {
return ExecutePower::invoke(DefaultQueueId, input_a, exponent, output_mem_config, output_tensor);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
parameter output_tensor is passed by value and only copied once; consider moving it to avoid unnecessary copies

Suggested change
return ExecutePower::invoke(DefaultQueueId, input_a, exponent, output_mem_config, output_tensor);
return ExecutePower::invoke(DefaultQueueId, input_a, exponent, output_mem_config, std::move(output_tensor));

ttnn/cpp/ttnn/operations/eltwise/unary/device/unary_composite_op.cpp
uint32_t exponent,
const std::optional<MemoryConfig>& output_mem_config,
std::optional<Tensor> output_tensor) {
return ExecutePower::invoke(DefaultQueueId, input, exponent, output_mem_config, output_tensor);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
parameter output_tensor is passed by value and only copied once; consider moving it to avoid unnecessary copies

Suggested change
return ExecutePower::invoke(DefaultQueueId, input, exponent, output_mem_config, output_tensor);
return ExecutePower::invoke(DefaultQueueId, input, exponent, output_mem_config, std::move(output_tensor));

ttnn/cpp/ttnn/operations/eltwise/unary/device/unary_composite_op.cpp
const Tensor& exponent,
const std::optional<MemoryConfig>& output_mem_config,
std::optional<Tensor> output_tensor) {
return ExecutePower::invoke(DefaultQueueId, input, exponent, output_mem_config, output_tensor);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ performance-unnecessary-value-param ⚠️
parameter output_tensor is passed by value and only copied once; consider moving it to avoid unnecessary copies

Suggested change
return ExecutePower::invoke(DefaultQueueId, input, exponent, output_mem_config, output_tensor);
return ExecutePower::invoke(DefaultQueueId, input, exponent, output_mem_config, std::move(output_tensor));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@ayerofieiev-tt ayerofieiev-tt Awaiting requested review from ayerofieiev-tt ayerofieiev-tt will be requested when the pull request is marked ready for review ayerofieiev-tt is a code owner

@dmakoviichuk-tt dmakoviichuk-tt Awaiting requested review from dmakoviichuk-tt dmakoviichuk-tt will be requested when the pull request is marked ready for review dmakoviichuk-tt is a code owner

@yan-zaretskiy yan-zaretskiy Awaiting requested review from yan-zaretskiy yan-zaretskiy will be requested when the pull request is marked ready for review yan-zaretskiy is a code owner

@patrickroberts patrickroberts Awaiting requested review from patrickroberts patrickroberts will be requested when the pull request is marked ready for review patrickroberts is a code owner

@eyonland eyonland Awaiting requested review from eyonland eyonland will be requested when the pull request is marked ready for review eyonland is a code owner

@rtawfik01 rtawfik01 Awaiting requested review from rtawfik01 rtawfik01 will be requested when the pull request is marked ready for review rtawfik01 is a code owner

@ttmtrajkovic ttmtrajkovic Awaiting requested review from ttmtrajkovic ttmtrajkovic will be requested when the pull request is marked ready for review ttmtrajkovic is a code owner

@rdjogoTT rdjogoTT Awaiting requested review from rdjogoTT rdjogoTT will be requested when the pull request is marked ready for review rdjogoTT is a code owner

At least 0 approving reviews are required to merge this pull request.

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

Successfully merging this pull request may close these issues.
2 participants
@KalaivaniMCW @rdjogoTT