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

New Operation: Fill_Tile_Pad ; Op to fill tile padding with a specific value #16785

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

yugi957
Copy link
Contributor

@yugi957 yugi957 commented Jan 15, 2025

Ticket

#16393

Problem description

Need to create an op that fills any tile padding with a particular value

What's changed

Only parallelizing across dims that aren't the last 2 for now. Simply iterate through the space between the logical shape and padded shape and fill with the respective value.

Checklist

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)

Copy link
Contributor

@sjameelTT sjameelTT left a comment

Choose a reason for hiding this comment

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

Rename the PR title to something more descriptive

@yugi957 yugi957 changed the title Atulk/fill pad New Operation: Fill_Pad ; Op to fill tile padding with a specific value Jan 16, 2025
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)

Copy link
Contributor

@bbradelTT bbradelTT left a comment

Choose a reason for hiding this comment

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

You should probably make the github actions error go away (either by changing the code or maybe adding a pragma).

Copy link
Contributor

@ntarafdar ntarafdar left a comment

Choose a reason for hiding this comment

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

A few comments. Overall great PR!

uint32_t padded_height = tt::div_up(height, tt::constants::TILE_HEIGHT) * tt::constants::TILE_HEIGHT;
uint32_t padded_width = tt::div_up(width, tt::constants::TILE_HEIGHT) * tt::constants::TILE_HEIGHT;
uint32_t tiles_per_2d_tensor =
padded_height / tt::constants::TILE_HEIGHT * padded_width / tt::constants::TILE_HEIGHT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of TT constants for tile height can you get it from the tensor, tile shape, @TT-BrianLiu , can you point us to the appropriate tile shape api.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd need to pass in an optional Tile parameter and plumb it through. You can look at struct Matmul's output_tile parameter in ttnn/cpp/ttnn/operations/matmul/device/matmul_op.hpp

I think it'd be better not to block this PR for adding that functionality.

@yugi957 yugi957 changed the title New Operation: Fill_Pad ; Op to fill tile padding with a specific value New Operation: Fill_Tile_Pad ; Op to fill tile padding with a specific value Jan 17, 2025
@yugi957
Copy link
Contributor Author

yugi957 commented Jan 17, 2025

Hi all, I'm trying to be more specific with what name we bind the op to for users, and these options came up:

  • fill_tile_padding
    • I'd like to go with this for now as this addresses the fact we are filling some kind of padding, but specifically implicit padding from tile alignment
  • fill_tile_alignment
    • I think this option is for if we decide it makes most sense not to consider alignment padding padding at all

and certainly open to more suggestions

@bbradelTT
Copy link
Contributor

Hi all, I'm trying to be more specific with what name we bind the op to for users, and these options came up:

  • fill_tile_padding
    • I'd like to go with this for now as this addresses the fact we are filling some kind of padding, but specifically implicit padding from tile alignment
  • fill_tile_alignment
    • I think this option is for if we decide it makes most sense not to consider alignment padding padding at all

and certainly open to more suggestions

A few more specific names could be something like

  • fill_implicit_tile_padding
  • fill_implicit_tile_alignment
  • fill_tile_alignment_padding (which I think you mentioned as well)

Anything with more words would probably be too long, although even with this length, most people will stay away from it.

I don't have a preference.

@ayerofieiev-tt you probably have the strongest opinion. What's your preference?

@yugi957
Copy link
Contributor Author

yugi957 commented Jan 17, 2025

  • fill_implicit_tile_padding
    I like this one personally!

Copy link
Contributor

@sjameelTT sjameelTT left a comment

Choose a reason for hiding this comment

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

For your next PR use these to get tile_shape/face_shape

const auto& tile_shape = input_tensor.get_tensor_spec().tile().get_tile_shape();
const auto& face_shape = input_tensor.get_tensor_spec().tile().get_face_shape();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants