[luci/pass] Add TaggedShapeAnalyzer::init() in RmUnnTransNetPass #14152
Conversation
This PR adds TaggedShapeAnalyzer::init() function. The init() function explicitly checks some conditions that have be met for the analyzer. ONE-DCO-1.0-Signed-off-by: seunghui youn <[email protected]>
zetwhite
force-pushed
the
1002/update-anayla
branch
from
f479894 to
d7c6618
Compare
| std::vector<int32_t> _in_shape; | ||
| std::vector<int32_t> _front_perm; | ||
| std::vector<int32_t> _mid_shape; | ||
| std::vector<int32_t> _back_perm; |
There was a problem hiding this comment.
this member will be initialized in init() and will used in other member functions.
I'm planning to change other methods like this - https://github.com/Samsung/ONE/pull/14150/files#r1784062781.
| template <loco::DataType DType> | ||
| bool TaggedShapeAnalyzer::init(const luci::CircleTranspose *front_transpose, | ||
| const luci::CircleReshape *mid_reshape, | ||
| const luci::CircleTranspose *back_transpose) |
There was a problem hiding this comment.
(note)
This is the main change - introduce this init() function to explicitly check the analyzer's working condition.
-
Before
- conditions(
c1andc2) are checked in callers and some conditions(c3) are not clearly checked
- conditions(
-
After
- all conditions are checked in this
init()function. - if
init()function fails it means that - the analyzer didn't support this case
- all conditions are checked in this
There was a problem hiding this comment.
Introducing init looks good to me. It's much clearer than before.
I think DType is not a good template parameter. It's the dtype of indices of reshape and dtype of indices of transpose. They don't have to be the same, so the checks in line 292 and 294 are too restrictive. One more concern is that users should call init to support S64 dtype, which looks verbose.
How about handling S32/S64 inside extract_const? It will simplify the function.
There was a problem hiding this comment.
Thank you very much for your detailed review!
I think DType is not a good template parameter.
I agree with your points.
How about handling S32/S64 inside extract_const? It will simplify the function.
Good idea. I updated this PR. Could you take another look?
zetwhite
force-pushed
the
1002/update-anayla
branch
from
3952db7 to
038e5a7
Compare
zetwhite
force-pushed
the
1002/update-anayla
branch
from
984bc99 to
4d0686e
Compare
zetwhite
force-pushed
the
1002/update-anayla
branch
from
4d0686e to
a46729d
Compare
| } | ||
| else if (dtype == loco::DataType::S64) | ||
| { | ||
| int64_t max_i32 = static_cast<int64_t>(std::numeric_limits<int32_t>::max()); |
There was a problem hiding this comment.
Please add #include <limits> to fix build error.
| auto rank = node->rank(); | ||
| for (auto i = 0u; i < rank; ++i) | ||
| { | ||
| uint32_t v = node->dim(i).value(); |
There was a problem hiding this comment.
Is it okay to not consider the 64-bit range?
| uint32_t v = node->dim(i).value(); | |
| uint64_t v = node->dim(i).value(); |
There was a problem hiding this comment.
@shs-park Thank you for the review!
loco::Dimension only holds uint32_t values. So we don't need to handle int64_t for this function.
This PR adds the TaggedShapeAnalyzer::init() function. The init() function explicitly checks some conditions that have to be met for the analyzer.
ONE-DCO-1.0-Signed-off-by: seunghui youn [email protected]