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

[Refactor] Remove all ctx parameters in memory operators #5862

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Critsium-xy
Copy link
Collaborator

@Critsium-xy Critsium-xy commented Jan 16, 2025

After Abacus start using template to implement caculation on different devices, the ctx parameter in operators became fully useless. But it is never removed because of its heavy usage. This pull request removes the ctx parameters in all memory operators.

Hint: If this pr is accepted, other pull requests may need rerun to ensure they are correct.

@Qianruipku
Copy link
Collaborator

It may be necessary to have a unified naming convention to distinguish whether this is a device or CPU resize_memory?

@Critsium-xy
Copy link
Collaborator Author

It may be necessary to have a unified naming convention to distinguish whether this is a device or CPU resize_memory?

In fact every type of operators have its own unique name defined by using keyword in memory_op.h, which clearly shows an operator's data type and device type. But no one uses that in fact. I used it in my new prs.

image

These new functions are also available for this

image

By using AbacusDevice_t you can clearly see which device this operator is operating, but this is merged yesterday so no one used it temporarily.

@Critsium-xy Critsium-xy marked this pull request as draft January 16, 2025 07:13
@Critsium-xy Critsium-xy changed the title [Refactor] Remove all ctx parameters in resize_memory_op [Refactor] Remove all ctx parameters in resize_memory_op and set_memory_op Jan 16, 2025
@Critsium-xy Critsium-xy marked this pull request as ready for review January 16, 2025 07:32
@AsTonyshment
Copy link
Collaborator

Nice work! I'm currently struggling with including things like this in my code:

Device* ctx = {};
base_device::DEVICE_CPU* cpu_ctx = {};

I was wondering what the hack exactly does this do? It seems that these pointers are no longer used after a total refactoring.

@Critsium-xy Critsium-xy marked this pull request as draft January 16, 2025 08:46
@Critsium-xy Critsium-xy marked this pull request as ready for review January 16, 2025 10:37
@Critsium-xy Critsium-xy changed the title [Refactor] Remove all ctx parameters in resize_memory_op and set_memory_op [Refactor] Remove all ctx parameters in memory operators Jan 16, 2025
@Qianruipku
Copy link
Collaborator

It may be necessary to have a unified naming convention to distinguish whether this is a device or CPU resize_memory?

In fact every type of operators have its own unique name defined by using keyword in memory_op.h, which clearly shows an operator's data type and device type. But no one uses that in fact. I used it in my new prs.

image

These new functions are also available for this

image

By using AbacusDevice_t you can clearly see which device this operator is operating, but this is merged yesterday so no one used it temporarily.

No one uses the using names defined in memory_op.h because they are not compatible with templates. Therefore, it is necessary to add extra using names in the definition of the template class. My suggestion is that although each class needs to include this using name, there should be a unified naming convention to distinguish between CPU and device, as well as between complex and real types. This would improve the readability of the code.

@mohanchen mohanchen added Refactor Refactor ABACUS codes GPU & DCU & HPC GPU and DCU and HPC related any issues The Absolute Zero Reduce the "entropy" of the code to 0 labels Jan 17, 2025
@Critsium-xy
Copy link
Collaborator Author

Exactly. I will soon add that suggested naming convention in the document later. But it seemed that this problem has no relationship with this pull request I think? Because the ctx parameter is completely useless as a parameter. Many codes even directly use a same "ctx" variable in every place where a ctx parameter is needed, no matter it is a gpu operator or cpu operator. I just removed it from parameter, not removing it from the template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GPU & DCU & HPC GPU and DCU and HPC related any issues Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants