[luci/pass] Introduce CanonicalizePass with Pad #13503
Conversation
| * @brief Class to canoncalize CircleNodes | ||
| * | ||
| */ | ||
| struct CanonicalizePass final : public logo::Pass |
There was a problem hiding this comment.
This pass name seems too general. This name more fits a stage, i.e., a sequence of passes (PTAL https://github.com/Samsung/ONE/pull/13503/files#r1690670613).
How about renaming to CanonicalizePaddingDtypePass, or simply ConvertS64PaddingToS32Pass ?
There was a problem hiding this comment.
Named this pass to do some multiple things as canonicalization.
This pass will run always without option.
| changed = true; | ||
| } | ||
|
|
||
| // TODO add more canonicalization |
There was a problem hiding this comment.
Putting all canonicalization tasks in a single pass seems to make this pass to have too many responsibilities.
There was a problem hiding this comment.
Yes, its' about canonicalization.
When there are too many things to do, someone can split to some categorize.
This will introduce CanonicalizePass with Pad.paddings canonicalize. ONE-DCO-1.0-Signed-off-by: SaeHie Park <[email protected]> Co-authored-by: Hyukjin Jeong <[email protected]>
seanshpark
force-pushed
the
luci_pass_canonicalize
branch
from
79fa6d0 to
65d19fd
Compare
There was a problem hiding this comment.
https://github.com/Samsung/ONE/pull/13503/files#r1690674624 violates single responsibility principle, which would leave a technical debt.
You can go as-is if you wish.
This will introduce CanonicalizePass with Pad.paddings canonicalize.