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

[Draft] attach extra tensor to tensor builder #13326

Closed
wants to merge 6 commits into from

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Jul 1, 2024

(WIP) This draft try to attach extra tensors to tensor builder.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn [email protected]

related : #13282

@zetwhite zetwhite force-pushed the 0626/attach-etensor branch 3 times, most recently from 13d4b96 to e332c24 Compare July 1, 2024 12:32
@zetwhite

This comment was marked as off-topic.

@zetwhite
Copy link
Contributor Author

zetwhite commented Jul 2, 2024

@nnfw-bot test onert-cross-release

@zetwhite zetwhite force-pushed the 0626/attach-etensor branch from cb7cca5 to e191a29 Compare July 2, 2024 05:08
@zetwhite zetwhite force-pushed the 0626/attach-etensor branch 2 times, most recently from e7fd283 to 0f65e6d Compare July 9, 2024 05:13
Comment on lines 878 to 889
// [idea2] : Generate Extra Tensors by traversing code_map

// Problem 1 :
// Since code_map lost its' backend context,
// it is hard to specify where estra tensor should be registered.

// Problme 2:
// Since ITrainableFunction is in exec area, it isn't a good idea to use member function of *Layer
// here, Istead of exec area interface, we have to implement another backend interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.

@hseok-oh
Copy link
Contributor

hseok-oh commented Jul 10, 2024

IMO

  • Introduce individual tensor manager (and memory manager if we need) for extra tensor
    • Extra tensor manager get request for extra tensor and manage extra tensor
    • Extra tensor manager may use extra memory manager to manage actual memory
  • Train backend's BackendContext owns extra tensor manager, and train kernel generator points extra tensor manager
  • When kernel generator visits each operator, visitor request extra tensor manager
    • Extra tensor manager return generated extra tensor(s): tensor struct with tensor info only, not allocated yet
    • Visitor create op's kernel layer and backward configure with extra tensor(s) from extra tensor manager
  • After kernel generator's visitor loop end, kernel generator request extra tensor manager to allocate in genKernels() (or generateFunctionMap())

ex.) CPU backend create tensor and allocate at once in genTensors(), but acl-cl/acl-neon/gpu-cl backends divide tensor creation and allocation.

Create tensors

// TODO Get compiler options from compiler, and use it rather than getting it from Env
if (util::getConfigString(util::config::EXECUTOR) == "Linear")
{
this->planTensors();
}
else
{
// For the executors that does not have fixed linear execution order:
// To make tensors never be deallocated, this is a workaround to use static memory planner
this->graph()->operands().iterate([&](const ir::OperandIndex &ind, const ir::Operand &) {
if (this->tensor_builder->isRegistered(ind))
this->tensor_builder->notifyFirstUse(ind);
});
}
this->tensor_builder->prepare();

Allocation after kernel generation

FunctionMap genKernels() override
{
FunctionMap ret;
// kernel_gen
for (auto &&op_ind : _data.op_order)
{
auto fn_seq = kernel_gen->generate(op_ind);
ret.emplace(op_ind, std::move(fn_seq));
}
tensor_builder->allocate();

@zetwhite
Copy link
Contributor Author

@hseok-oh Thank you a lot for taking a look 😄

I catched your point. It looks quite good 👍 😄

Since I already implemented ExtraTensorManage and ExtraTensorIndex .. etc, I could quickly implement your suggestions. I'll revise it right now.

@zetwhite zetwhite force-pushed the 0626/attach-etensor branch from 0f65e6d to fd174ee Compare July 16, 2024 05:49
zetwhite added 3 commits July 17, 2024 19:19
(WIP) This draft try to attach extra tensors to tensor builder.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn <[email protected]>
@zetwhite zetwhite force-pushed the 0626/attach-etensor branch from a6ed87c to 4e9ec7e Compare July 17, 2024 10:19
@zetwhite zetwhite force-pushed the 0626/attach-etensor branch from b37741f to f7c052d Compare July 19, 2024 03:08
{
assert(_mem_planner->memory_plans().find(ind) != _mem_planner->memory_plans().end());
const auto &mem_blk = _mem_planner->memory_plans().at(ind);
return _mem_alloc->base() + mem_blk.offset;
}

// specialization
Copy link
Contributor

@ragmani ragmani Jul 22, 2024

Choose a reason for hiding this comment

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

It's template instantiation, not specialization. I'm not sure if it's template instantiation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I have a question.

If template member functions are defined in a cc file, can these instantiated template class be used in other cc files without including this cc file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If template member functions are defined in a cc file, can these instantiated template class be used in other cc files without including this cc file?

Yes, I don't know why. But this worked well.
I'd better check accurate C++ grammar related to this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very surprised that it works. I'm curious about the reason why there is no compilation problem.
However, I'm worried about multiple definition problem if those instantiated template classes are used in multiple cc files. Could you check that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm worried about multiple definition problem if those instantiated template classes are used in multiple cc files. Could you check that too?

That's why I put instantiation here.
I first tried it in the header, but the compiler complains about multiple definitions.

I'm very surprised that it works. I'm curious about the reason why there is no compilation problem

I couldn't fully understand why it works. But I could find similar answers in stack overflow :
https://stackoverflow.com/questions/32707522/explicit-instantiation-of-template

Copy link
Contributor

Choose a reason for hiding this comment

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

I first tried it in the header, but the compiler complains about multiple definitions.

Usually, we have resolved by using inline keyword. Anyway, it seems that there is no complaint about multiple definition with the template instantiation.

I couldn't fully understand why it works. But I could find similar answers in stack overflow :
https://stackoverflow.com/questions/32707522/explicit-instantiation-of-template

I also found a related answers in https://isocpp.org/wiki/faq/templates#separate-template-fn-defn-from-decl.

@zetwhite zetwhite closed this Jul 23, 2024
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