Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[onert-micro] Reduce duplicate code in pool layers #12428

Merged

Conversation

ragmani
Copy link
Contributor

@ragmani ragmani commented Jan 9, 2024

This commit reduces duplicate code in pool layers.

  • Introduce Pool2DCommon.h that has common functions for pool layers.
    • configure_kernel_CirclePool2DCommon : unifies configure function of pool layers.
    • createPoolParams : unifies creating parameters of pool layers.
  • Apply common function for pool layers.

ONE-DCO-1.0-Signed-off-by: ragmani ragmani0216@gmail.com


For #11744

@ragmani ragmani force-pushed the onert-micro/Reduce_DC_pool_layers branch from 2b48924 to 0c74805 Compare January 9, 2024 11:17
@ragmani ragmani added PR/NO MERGE Please don't merge. I'm still working on this :) and removed PR/NO MERGE Please don't merge. I'm still working on this :) labels Jan 9, 2024
This commit reduces duplicate code in pool layers.
  - Introduce Pool2DCommon.h that has common functions for pool layers.
    - configure_kernel_CirclePool2DCommon : unifies configure function of pool layers.
    - createPoolParams : unifies creating parameters of pool layers.
  - Apply common function for pool layers.

ONE-DCO-1.0-Signed-off-by: ragmani <ragmani0216@gmail.com>
@ragmani ragmani force-pushed the onert-micro/Reduce_DC_pool_layers branch from 0c74805 to 63d9907 Compare January 9, 2024 12:24
@ragmani ragmani added the PR/ready for review It is ready to review. Please review it. label Jan 9, 2024
Comment on lines 56 to 57
inline luci_interpreter_pal::PoolParams createPoolParams(const circle::Operator *cur_op,
BaseRuntimeGraph *runtime_graph)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to pass const kernels::SISOKernel &siso_kernel parameter here so as not to read the input and output tensor twice, since this has already been done in the calling function.
What do you think?)

Copy link
Contributor Author

@ragmani ragmani Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to pass const kernels::SISOKernel &siso_kernel instead of BaseRuntimeGraph *runtime_graph? I think it looks better.

Copy link
Contributor

@BalyshevArtem BalyshevArtem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything apart from one small comment looks great, thanks)

Comment on lines 59 to 63
const auto input_index = cur_op->inputs()->operator[](0);
const auto output_index = cur_op->outputs()->operator[](0);

assert(input_index != -1);
assert(output_index != -1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, now input_index and output_index can be removed, I think

@ragmani ragmani force-pushed the onert-micro/Reduce_DC_pool_layers branch from 90562d3 to 8b45469 Compare January 11, 2024 12:38
Copy link
Contributor

@BalyshevArtem BalyshevArtem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@BalyshevArtem
Copy link
Contributor

@chunseoklee, PTAL)

@BalyshevArtem BalyshevArtem merged commit bf7acae into Samsung:master Jan 17, 2024
4 checks passed
@ragmani ragmani deleted the onert-micro/Reduce_DC_pool_layers branch July 3, 2024 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants