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

Working with the Reorder #1980

Open
CodeCrafter18 opened this issue Jun 27, 2024 · 9 comments
Open

Working with the Reorder #1980

CodeCrafter18 opened this issue Jun 27, 2024 · 9 comments

Comments

@CodeCrafter18
Copy link

Hello!
I have a question regarding the usage of reorders.
For example, let's consider ref_sum.hpp.

if (need_output_reorder()) {
    CHECK(reorder_primitive_desc_create(
		reorder_pds_[n_], engine, dst_acc_md(), dst_md()));
}

In this case, the following function will be called:

status_t reorder_primitive_desc_create(std::shared_ptr<primitive_desc_t> &pd,
        engine_t *engine, const memory_desc_t *src_md,
        const memory_desc_t *dst_md, const primitive_attr_t *attr) {
    return reorder_primitive_desc_create(
            pd, engine, src_md, engine, dst_md, engine, attr);
}

However, in this scenario, create_resource(...) is never called.
If I want to write my own Reorder that overrides the create_resource(...) function,
as it's done in ACL, then I'll get a runtime error when I call ctx.get_resource_mapper()...

How can I work around this problem if my implementation uses resource_mapper?

Also, I have a question about why the Sum, Reorder, and Concat operations have separate logic, for example, for obtaining the list of implementations?​​​​​​​​​​​​​​​​

Thanks.

@densamoilov
Copy link
Contributor

Hi @CodeCrafter18,

First of all, primitive descriptor has nothing to do with create_resource(...) and resource_mapper. It is the primitive that utilizes those.
The primitive_t::create_resource is always called at the primitive initialization stage.
So you just need to override create_resource(...) in your primitive and then use the resource_mapper from the execution context during the execution step.

@CodeCrafter18
Copy link
Author

Hi @densamoilov,

As I pointed out above in a specific example (ref_sum). The “create” function will be called, but the overridden “create_resource” is not. As a result, the “resource_mapper” is empty at the execution stage. One way to get around this is to create an instance again in "execute" in the case of an “empty” for “resource_mapper”, but this looks like some kind of “crutch”.

@densamoilov
Copy link
Contributor

@CodeCrafter18, in the example above you mentioned primitive descriptor and not the primitive, that's why I'm still confused.
One thing to keep in mind is that create_resource is ALWAYS called from the main primitive. If the main primitive has nested primitives then it is the main primitive's responsibility to call create_resource from the nested primitives. Another thing to keep in mind is that the resource mapper in the execution context object (exec_ctx_t) that is passed to the execute function of the main primitive should be passed down to the nested primitives. In order to do that you need to create a new execution context for the nested primitives using the execution context for the main primitive:

exec_ctx_t r_ctx(ctx, std::move(r_args));

@CodeCrafter18
Copy link
Author

Let's do it again.

We are looking at a specific example: (oneDNN/src/cpu/ref_sum.hpp)

The init(...) function line 55-58:

if (need_output_reorder()) {
    CHECK(reorder_primitive_desc_create(
		reorder_pds_[n_], engine, dst_acc_md(), dst_md()));
}

In this case, the Reorder is selected and created from the general list.
My implementation uses create_resource.
But in this test, I never call create_resource.
Therefore, at the execute stage, I encounter an error.

For regular reorder launches, for example, via benchen, there is no such problem.

@densamoilov
Copy link
Contributor

If you want to create a resource for your reorder implementation that is used in ref_sum.hpp then you will need to add ref_sum_t::create_resource(…) that would call create_resource from each element of the reorders_ vector.

@CodeCrafter18
Copy link
Author

CodeCrafter18 commented Jul 3, 2024

Well, thank you. Why hasn't it been done already? Now the ACL has its own implementation for the Reorder operation and can be called. If it's not difficult, could you provide a modification for test_sum from ctest?

And what about the second question, why is there different logic for Reorder Sum and Concat?

@densamoilov
Copy link
Contributor

densamoilov commented Jul 3, 2024

Why hasn't it been done already?

It's a bug in the case when ACL reorder is available. The ref_sum_t class needs to have this:

#if DNNL_AARCH64 && DNNL_AARCH64_USE_ACL
status_t create_resource(
engine_t *engine, resource_mapper_t &mapper) const override {
CHECK(conv_p_->create_resource(engine, mapper));
return status::success;
}
#endif

+@jondea

@CodeCrafter18
Copy link
Author

It looks like simple_layer_normalization has the same problem.

@jondea
Copy link
Contributor

jondea commented Jul 5, 2024

I've made an issue to look into this. We also have work in progress to remove the use for create_resource for the ACL implementations, which I think should fix this issue.

@vpirogov vpirogov assigned jondea and unassigned densamoilov Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants