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

Add TTNN Dialect guidelines #1785

Merged
merged 3 commits into from
Jan 17, 2025
Merged

Add TTNN Dialect guidelines #1785

merged 3 commits into from
Jan 17, 2025

Conversation

svuckovicTT
Copy link
Contributor

Following last year's last weekly tech talk, I'm adding docs on TTNN dialect contribution guidelines. This docs is a subset of the discussed doc - it doesn't include sections on generalizing ttnn->emitc conversions, and runtime ops.

@@ -0,0 +1 @@
# TTNN Interactive Visualizer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mdbook auto-created this when I ran it. Looks like there's a link to this doc in SUMMARY.md but the doc didn't exist. @nsmith would you like me to remove the link and/or this newly added file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind removing the file? It's kind of hacky, but the interactive visualizer is already html so just by naming it correctly the SUMMARY.md generation ends up finding it. I think this MD would now generate new html that would clobber the interactive visualizer.

Copy link
Contributor

@nsmithtt nsmithtt left a comment

Choose a reason for hiding this comment

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

Please remove that 1 MD file, otherwise looks great, more docs ftw!

Copy link
Contributor

@sdjordjevicTT sdjordjevicTT left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @svuckovicTT!

OptionalAttr<FloatAttr>:$pad_value);
```

Mixing types and attributes within the ordering is **not** an issue, this is valid:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about "types and attributes", technically it should be "operands and attributes" if we follow the MLIR glossary, but "operands" is a bit overloaded, so I'm not sure what's the best term here.

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 see "operand" as a kind of higher level abstraction term, whereas Type and Attr are actual defined types in cpp, which is why I use those terms.


There were very few issues with these previously, and they were caused by inconsistencies in TTNN lib APIs.

### Attrs vs Types
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe one important point about attrs is constness, if we can reasonably assume that some value will always be known in compile time, than it's probably a good candidate for attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point, let me add that!

@svuckovicTT svuckovicTT force-pushed the svuckovic/docs-ttnn-dialect branch from cbc074d to ba5f7fe Compare January 17, 2025 13:58
@svuckovicTT svuckovicTT merged commit 74470de into main Jan 17, 2025
23 checks passed
@svuckovicTT svuckovicTT deleted the svuckovic/docs-ttnn-dialect branch January 17, 2025 15:48
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.

4 participants