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] Introduce ExtraTensorIndex #13605

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Aug 7, 2024

This PR introduces ExtraTensorIndex.
ExtraTensorIndex will be used to identify extra tensor in tensor registry.

ONE-DCO-1.0-Signed-off-by: seunghui youn [email protected]

draft : #13486
related : #13282

@zetwhite zetwhite marked this pull request as ready for review August 7, 2024 09:16
@zetwhite zetwhite added approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it. labels Aug 7, 2024
@zetwhite zetwhite requested a review from a team August 7, 2024 09:16
@glistening
Copy link
Contributor

glistening commented Aug 7, 2024

When I read ExtraTensorIndex, it was hard at least for me what ExtraTensor does mean.
To figure out the meaning of ExtranTensor, I searched and found the answer in #13576.

ExtraTensor is a tensor that is accessed within one operation layer.
In other words, the scope of the extra tensor is confined to one specific layer.

It would be better to use some terms in your explanation above.
(e.g. LayerScopeTensor, LayerTemporaryTensor, ...)

(ADD)

@zetwhite told me that I am not the only one who was curious of the meaning of Extra#13604 (comment)

@zetwhite
Copy link
Contributor Author

zetwhite commented Aug 8, 2024

It would be better to use some terms in your explanation above. (e.g. LayerScopeTensor, LayerTemporaryTensor, ...)

Thansk for comments! I also feel awkward with the name 'Extra'.
@ys44kim asked me similar a questions(#13604 (comment)) and I feel hard to explain where the name comes from.

The name LayerTemporaryTensor looks good to me. I will rename it soon.

(+) About renaming it, I will do it at once, after both this PR and #13604 are merged.

jyoungyun
jyoungyun previously approved these changes Aug 8, 2024
Copy link
Contributor

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@jyoungyun jyoungyun requested a review from a team August 8, 2024 02:56
@glistening
Copy link
Contributor

@zetwhite or who is going to merge this,

Please update a typo in commit message.

ExtraTensorIndex will be used to indentify identify extra tensor in tensor registry.

This PR introduces ExtraTensorIndex.
ExtraTensorIndex will be used to identify extra tensor in tensor registry.

ONE-DCO-1.0-Signed-off-by: seunghui youn <[email protected]>
@zetwhite zetwhite requested a review from jyoungyun August 8, 2024 03:33
@zetwhite
Copy link
Contributor Author

zetwhite commented Aug 8, 2024

Please update a typo in commit message.

Ah, Nice catch. I updated.
Thank you

Comment on lines +76 to +77
static_assert(sizeof(size_t) >= sizeof(uint32_t),
"ExtraTensorIndex's hash creation error, size_t size is less than uint32_t");
Copy link
Contributor

@glistening glistening Aug 8, 2024

Choose a reason for hiding this comment

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

(optional) Personally, I prefer to remove this assert.

In general, sizeof(size_t) is 4 on 32-bit system, 8 on 64-bit system. Both cases, it is >= 4 (= sizeof(uint32_t)).

If any, I don't think onert will support such kind of system.

Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@glistening glistening merged commit 036cb5d into Samsung:master Aug 8, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: 2 Require at least 2 approvals 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.

4 participants