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] Propagate shared memory operand indexes to cpu backend #14230

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mbencer
Copy link
Contributor

@mbencer mbencer commented Oct 16, 2024

This commit adds propagation of shared memory operand indexes to cpu backend. Note that the propagated indexes map is not filled yet.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com

Draft: #14057

This commit adds propagation of shared memory operand indexes to cpu backend.
Note that the propagated indexes map is not filled yet.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com
Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

This PR changes the interface between the core and the backend. The code seems fine, but I'm not 100% sure about the direction (or details). It's complicated for me to review this.

And I'm also worried that you might have to work twice because there are different opinions between me and other members of onert. For this part, I would like to hear others' opinions first. (maybe @ragmani or @hseok-oh)

@ragmani
Copy link
Contributor

ragmani commented Oct 31, 2024

This PR changes the interface between the core and the backend. The code seems fine, but I'm not 100% sure about the direction (or details). It's complicated for me to review this.

@zetwhite
I couldn't find the part that changes the interface. Could you explain in more detail what you worried about?

@zetwhite
Copy link
Contributor

zetwhite commented Nov 1, 2024

@ragmani

I couldn't find the part that changes the interface.

I thought that virtual BackendContext::sharedMemoryOperandIndexes() is newly introduced.

Could you explain in more detail what you worried about?

I'm fine with virtual BackendContext::sharedMemoryOperandIndexes() :)

But I'm not sure why builtin::TensorBuilder has to be changed in this PR. I thought basic::TensorBuilder.h and builtin::TensorBuilder.h had no dependency.

@ragmani
Copy link
Contributor

ragmani commented Nov 1, 2024

I thought that virtual BackendContext::sharedMemoryOperandIndexes() is newly introduced.

I realized what interface changed.

I'm fine with virtual BackendContext::sharedMemoryOperandIndexes() :)
But I'm not sure why builtin::TensorBuilder has to be changed in this PR. I thought basic::TensorBuilder.h and builtin::TensorBuilder had no dependency.

I guess sharedMemoryOperandIndexes was introduced because of the basic::TensorBuilder.h modification.
@mbencer I would like to implement the In-place functionality as an independent module(like function without modifying the existing for loop) but I don't have time to look through your code carefully now. Could you copy your changes into cpu backend instead of the basic::TensorBuilder.h modification although it would cause code duplication? If so, you could remove BackendContext::sharedMemoryOperandIndexes().

@mbencer
Copy link
Contributor Author

mbencer commented Nov 4, 2024

@zetwhite @ragmani

I thought basic::TensorBuilder.h and builtin::TensorBuilder.h had no dependency

Unfortunately, they are dependent via static polymorphism (despite the lack common interface) in genTensors where ctx a passed as an template argument

But I'm not sure why builtin::TensorBuilder has to be changed in this PR. I thought basic::TensorBuilder.h and builtin::TensorBuilder.h had no dependency.

I see, after modification of BackendContextHelpers.h I was able to revert changes in builtin.

I would like to implement the In-place functionality as an independent module(like function without modifying the existing for loop)

As I understand you propose to calculate indexes of shared memory operands during each usage? I see advantages of such approach (limited scope of changes) but please also review the current approach - note that shared_memory_operand_idx param is passed to genTensors and initConsts but not used yet (postponed to the separate PR).

If so, you could remove BackendContext::sharedMemoryOperandIndexes()

The scope is limited now to backend/cpu.

@mbencer mbencer requested a review from zetwhite November 5, 2024 12:25
Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

LGTM

@mbencer
Copy link
Contributor Author

mbencer commented Nov 14, 2024

FYI: I will be on vacation until 26 (Nov) with excluding Wed (20 Nov) - in this day I am going to work. Please expect delays in communication ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants