[luci/service] Migrate shape inferences for Conv2D ops #14125
Conversation
This commit migrates shape inference of Conv2D and DepthwiseConv2D to sinf::Algorithm. ONE-DCO-1.0-Signed-off-by: Chansu Park <[email protected]>
|
Related : #14115 This just migrates these functions and separates to dedicated file.
It seems more verbose if I split this... |
pcs1265
added
SSAFY
PR/ready for review
| */ | ||
|
|
||
| #include "Check.h" | ||
| #include "CircleShapeInferenceHelper.h" |
There was a problem hiding this comment.
| #include "CircleShapeInferenceHelper.h" |
Is this header used..?
There was a problem hiding this comment.
Oh, I missed that.
Since I just migrated them, it doesn't need to be included this time.
It should be included in the next PR.
Thank you for checking.
| #include "HelperConv2Ds.h" | ||
| #include "luci/Service/CircleShapeInference.h" | ||
|
|
||
| #include "CircleCloneNode.h" | ||
| #include "CircleShapeInferenceHelper.h" | ||
|
|
||
| #include <luci/Log.h> |
There was a problem hiding this comment.
I thought there was a rule for header including order.
I'm not sure if it follows the rule or not. 🤔
I guess there was a comment about the rule, but I couldn't remember where it is.
There was a problem hiding this comment.
+) Ah, here it is - #13784 (comment)
There was a problem hiding this comment.
Header including order have to be changed like this :
| #include "HelperConv2Ds.h" | |
| #include "luci/Service/CircleShapeInference.h" | |
| #include "CircleCloneNode.h" | |
| #include "CircleShapeInferenceHelper.h" | |
| #include <luci/Log.h> | |
| #include "luci/Service/CircleShapeInference.h" | |
| #include "CircleCloneNode.h" | |
| #include "CircleShapeInferenceHelper.h" | |
| #include "HelperConv2Ds.h" | |
| #include <luci/Log.h> |
| @@ -14,7 +14,11 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| #include "HelperConv2Ds.h" | |||
There was a problem hiding this comment.
(optional)
How about renaming a header file to Conv2DHelepr.h
I found out that there are more headers named *.Helper.h than Helper*.h.
So I thought the name *Helper.h might be good for consistency.
ONE/compiler [$?] …
➜ find -name '*Helper.h' | wc -l
36
ONE/compiler [$?] …
➜ find -name 'Helper*.h' | wc -l
10But still, it is a personal preference, So I don't want to force it at all.
Please feel free to make your own decision.
There was a problem hiding this comment.
(optional 2)
It would be also good to move the infer_conv2d_type helper function to the CircleShapeInferenceHelper.h header file directly, for the reusability of helper functions.
As I know, because it is a template function, it is okay that its definition can be located in the header file.
There was a problem hiding this comment.
As I know, because it is a template function, it is okay that its definition can be located in the header file.
Ah, there was a history about HelperPads introduced. If so, your suggestion looks good for me :)
| @@ -14,7 +14,11 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| #include "HelperConv2Ds.h" | |||
There was a problem hiding this comment.
(optional 2)
It would be also good to move the infer_conv2d_type helper function to the CircleShapeInferenceHelper.h header file directly, for the reusability of helper functions.
As I know, because it is a template function, it is okay that its definition can be located in the header file.
|
I apologize for the delay. I've been busy preparing for the SSAFY project presentation. |
AFAIK, it isn't urgent.
Yes, Please. Thank you for your work:) |
This commit migrates shape inferences of Conv2D and DepthwiseConv2D to sinf::Algorithm.
ONE-DCO-1.0-Signed-off-by: Chansu Park [email protected]