TTNN->EmitC new op conversion infrastructure #2331
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
azecevicTT
requested review from
svuckovicTT,
sdjordjevicTT,
mtopalovicTT,
jserbedzijaTT and
nsmithtt
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
sdjordjevicTT
approved these changes
Feb 28, 2025
- TODO: add gtest to confirm expectations
- TODO: add tests for std::array - TODO: add tests for std::variant
azecevicTT
force-pushed
the
azecevic/emitc-emitter
branch
from
bd880d8 to
0829d58
Compare
svuckovicTT
requested changes
Mar 3, 2025
|
@svuckovicTT For the docs I planned to do it in the separate PR, just to test it a little bit further to verify that this won't require some major change. |
svuckovicTT
approved these changes
Mar 3, 2025
azecevicTT
added a commit
that referenced
this pull request
Mar 6, 2025
### Ticket #2343 ### Problem description Part of the effort to onboard all TTNN ops to conversion infrastructure introduced in #2331 ### What's changed Transitioned ops with existing conversion to conversion with the new converter. Few changes in the Emitter class, mainly in the way how operands are treated, to accommodate more complex cases like `Variadic` of tensors that maps to `std::vector<ttnn::Tensor>`.
vwellsTT
pushed a commit
that referenced
this pull request
Mar 6, 2025
### Ticket #2343 ### Problem description Part of the effort to onboard all TTNN ops to conversion infrastructure introduced in #2331 ### What's changed Transitioned ops with existing conversion to conversion with the new converter. Few changes in the Emitter class, mainly in the way how operands are treated, to accommodate more complex cases like `Variadic` of tensors that maps to `std::vector<ttnn::Tensor>`.
odjuricicTT
pushed a commit
that referenced
this pull request
Mar 8, 2025
### Ticket #2311 ### Problem description Existing way of TTNN->EmitC conversion exhibits a few limitations: 1. requires too much consideration on the OP to OP basis (requires user to explicitly make differentiation between operands and attributes) 2. didn't take into consideration many-to-many relationship of MLIR and C++ types (i.e. {`ArrayAttr`, `DenseI32ArrayAttr`} <-> {`ttnn::SmallVector<int32_t>`, `std::vector<uint32_t>`}) ### What's changed Introduction of the new `EmitCTTNNEmitter` class that should address these limitations in the following way: 1. Most conversions should follow this pattern ```cpp ttnn_to_emitc::EmitCTTNNEmitter<TTNNOp> emitter(srcOp, adaptor, rewrite); llvm::SmallVector<mlir::Attribute> args { emitter.emit(srcOp.getArg0()), emitter.emit(srcOp.getArg1()), ..., emitter.emit(srcOp.getArgN()) }; emitter.replaceOp(*this, args); return success(); ``` 2. For many-to-many relationship between MLIR and C++ types we 'hint' emit member function with the target C++ type ```cpp emitter.emit<ttnn::SmallVector<int32_t>>(srcOp.getSomeDenseArrayAttr()) ``` Of course, this is still a utility and it's not baked directly into MLIR's conversion pattern rewriter, so we can still do it the old way for the ops whose structure deviates too much from the general op structure. ### Checklist - [x] New/Existing tests provide coverage for changes - At this point `EmitCTTNNEmitter` and related class should still be considered WIP, next goal is to transfer all of the existing ops to this type of conversion and leverage existing tests to iron out some missing details
odjuricicTT
pushed a commit
that referenced
this pull request
Mar 8, 2025
### Ticket #2343 ### Problem description Part of the effort to onboard all TTNN ops to conversion infrastructure introduced in #2331 ### What's changed Transitioned ops with existing conversion to conversion with the new converter. Few changes in the Emitter class, mainly in the way how operands are treated, to accommodate more complex cases like `Variadic` of tensors that maps to `std::vector<ttnn::Tensor>`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#2311
Problem description
Existing way of TTNN->EmitC conversion exhibits a few limitations:
ArrayAttr,DenseI32ArrayAttr} <-> {ttnn::SmallVector<int32_t>,std::vector<uint32_t>})What's changed
Introduction of the new
EmitCTTNNEmitterclass that should address these limitations in the following way:emitter.emit<ttnn::SmallVector<int32_t>>(srcOp.getSomeDenseArrayAttr())Of course, this is still a utility and it's not baked directly into MLIR's conversion pattern rewriter, so we can still do it the old way for the ops whose structure deviates too much from the general op structure.
Checklist
EmitCTTNNEmitterand related class should still be considered WIP, next goal is to transfer all of the existing ops to this type of conversion and leverage existing tests to iron out some missing details