Override mechanism #1417
Override mechanism #1417
Conversation
vcanicTT
requested review from
sdjordjevicTT,
svuckovicTT,
mtopalovicTT,
rpavlovicTT,
nobradovictt,
jserbedzijaTT,
jnie-TT and
nsmithtt
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
test/unittests/TestOptimizerOverrides/TestOptimizerOverrides.cpp
Outdated
Show resolved
Hide resolved
vcanicTT
force-pushed
the
vcanic/override_mechanism
branch
from
6c756cc to
2f02d8c
Compare
vcanicTT
force-pushed
the
vcanic/override_mechanism
branch
from
2f02d8c to
295dc19
Compare
|
|
||
| private: | ||
| // Flags for enabling/disabling the optimizer passes | ||
| bool enableOptimizer = true; |
There was a problem hiding this comment.
From where did you pull these defaults?
There was a problem hiding this comment.
If you pass any override for optimizer pass, optimizer should be enabled.
There was a problem hiding this comment.
Maybe we should throw error instead if you do that(passing in override with optimizer set to false) and align defaults with current defaults in code?
There was a problem hiding this comment.
Can we pull defaults from the pass Options themselves?
There was a problem hiding this comment.
I think, the answer is the same like here, #1417 (comment)
| // override-output-layout=op1=2x2:dram:interleaved:tile:fp32,op2=4x4:l1:block_sharded:row_major:fp16 | ||
| // Example: | ||
| // override-output-layout=add_1_2=1x1:dram:interleaved:row_major:f32" | ||
| if (outputLayoutOverrides.size() > 0) { |
There was a problem hiding this comment.
Could you reuse OutputLayoutOverrideParser print method? Same applies for inputLayoutOverrides.
There was a problem hiding this comment.
I think I need to change a little bit the method. I can write for each segment separate method and call it from toString.
| case mlir::tt::ttnn::TensorMemoryLayout::None: | ||
| options += "none"; | ||
| break; | ||
| case mlir::tt::ttnn::TensorMemoryLayout::Interleaved: |
There was a problem hiding this comment.
Do these attributes have toString or print methods? Would scale much better, similar for types below.
There was a problem hiding this comment.
I'll check, if they don't exist I'll add. Also, this related to the answer on the previous question.
There was a problem hiding this comment.
Look into OutputLayoutOverrideParser::print
| ASSERT_EQ(optimizerOverridesHandler.getMeshShape()[1], meshShape[1]); | ||
| } | ||
|
|
||
| // Test the toString method |
There was a problem hiding this comment.
This is the only true test here, rest are bit pointless. In rest of them you set some attr in memory on the class and then compare getter of the class and the source attr, imo that does not make sense.
There was a problem hiding this comment.
I would like we test everything what we write
There was a problem hiding this comment.
I fully support that but would you call this a meaningful unit test:
x = 5;
assert(x == 5);
?
I mean some of them are setting non POD structs so you can leave them as well but for flags and ints Im not sure whats the value.
There was a problem hiding this comment.
Or just pack all POD ones in a single test case.
There was a problem hiding this comment.
Yes, it is meaningful test, especially if you work with floating points or registers, or in some concurrency mode, I don't know what the context around this example is.
Also, this is more like explicit statement, what I test here is interface, so I see only functions to be tested, when I write unittests I always try to separate process of developing and testing in my head.
If you still think all setters and getters should be inside one test function I can do that, but I think there is a lot functions, and probably in the future it will be more.
| case mlir::tt::ttnn::TensorMemoryLayout::None: | ||
| options += "none"; | ||
| break; | ||
| case mlir::tt::ttnn::TensorMemoryLayout::Interleaved: |
There was a problem hiding this comment.
Look into OutputLayoutOverrideParser::print
|
|
||
| private: | ||
| // Flags for enabling/disabling the optimizer passes | ||
| bool enableOptimizer = true; |
There was a problem hiding this comment.
Can we pull defaults from the pass Options themselves?
| std::string options = ""; | ||
|
|
||
| if (enableOptimizer) { | ||
| options += "enable-optimizer=true "; |
There was a problem hiding this comment.
Is it possible to read the string values of options directly from TTIRToTTNNBackendPipelineOptions?
There was a problem hiding this comment.
I don't think we need new structures for overrides, everything is related to Optimizer, so it is optimizer override.
There was a problem hiding this comment.
Probably we can extract names and defaults, maybe I can instantiate object of TTIRToTTNNBackendPipelineOptions struct, and get what I need.
vcanicTT
force-pushed
the
vcanic/override_mechanism
branch
from
47e7e40 to
a8b19cf
Compare
| @@ -2,53 +2,106 @@ | |||
| // | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| #pragma once | |||
There was a problem hiding this comment.
Is this really needed?
| inputLayoutOverrides[opName] = params; | ||
| } | ||
| void OptimizerOverridesHandler::addInputLayoutOverride( | ||
| StringRef opName, SmallVector<int64_t> operandIdxes) { |
There was a problem hiding this comment.
Vectors should be passed in by ref.
| #include "ttmlir/Dialect/TT/IR/TTOpsTypes.h" | ||
| #include "ttmlir/Dialect/TTNN/IR/TTNNOpsAttrs.h" | ||
| #include "ttmlir/Dialect/TTNN/IR/TTNNOpsTypes.h" | ||
|
|
||
| namespace mlir::tt::ttnn { |
There was a problem hiding this comment.
Can you create a new file in Utils directory for this instead of merging into this general existing one?
There was a problem hiding this comment.
Something like PassOptions.h or PassOverrides.h, same comment for cpp.
| @@ -2,11 +2,13 @@ | |||
| // | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| #pragma once | |||
|
|
|||
| #ifndef TTMLIR_DIALECT_TTNN_PIPELINES_TTNNPIPELINES_H | |||
| #define TTMLIR_DIALECT_TTNN_PIPELINES_TTNNPIPELINES_H | |||
There was a problem hiding this comment.
code/includes outside of area guarded by header guard; consider moving it
| @@ -2,53 +2,106 @@ | |||
| // | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| #pragma once | |||
|
|
|||
| #ifndef TTMLIR_DIALECT_TTNN_UTILS_OPTIMIZEROVERRIDES_H | |||
| #define TTMLIR_DIALECT_TTNN_UTILS_OPTIMIZEROVERRIDES_H | |||
There was a problem hiding this comment.
code/includes outside of area guarded by header guard; consider moving it
vcanicTT
force-pushed
the
vcanic/override_mechanism
branch
from
a8b19cf to
bdcde2f
Compare
Initial version of framework for overriding the models.