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

Preserve the executor in generation #1128

Closed
wants to merge 1 commit into from
Closed

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Oct 3, 2022

This PR will preserve the executor of given LinOp in generation.
clone still moves everything to the same executor.

  • [ ]: should the clone use is_accessible not the pointer?

@yhmtsai yhmtsai self-assigned this Oct 3, 2022
@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. type:solver This is related to the solvers labels Oct 3, 2022
@upsj
Copy link
Member

upsj commented Oct 3, 2022

Seems like you are missing some diffs here. If I understand the intention correctly, I also tend to disagree with it - this was explicitly changed in #1037 and #753 to address #301
What use case do you have in mind for this change?

@yhmtsai
Copy link
Member Author

yhmtsai commented Oct 3, 2022

In the beginning, I just thought we give what user want.
I consider the mixed executor is the user desired way, but you might consider it is a wrong usage.

I also thought about the multigrid mixing GPU and CPU usage.
It's not the same situation though.

mg.with_mg_level(coarsen::build().on(gpu), coarsen::build().on(cpu))

if we would like to allow the above mixing usage, what scope should we force to use the same executor?
For me, I can not distinguish the role of the mg_level and system matrix because they are components of LinOp.

@upsj
Copy link
Member

upsj commented Oct 3, 2022

My suggestion at the time of the value semantics PR was making the cross-executor copy explicit by putting it into a PreserveExecutor LinOp(Factory) that also avoids the temporary allocation this approach would require at each cross-executor apply call.

@yhmtsai
Copy link
Member Author

yhmtsai commented Oct 5, 2022

I mean it introduces the copy in creation not during the copy. .on is already quite specific request for me.

@upsj
Copy link
Member

upsj commented Oct 5, 2022

To recap my arguments from last time:

  • Relying on the implicit temporary copy for a repeated apply comes with a performance penalty due to repeated allocations and deallocations
  • on can't be a strong statement of intent if it is mandatory. I have some plans to provide functionality to avoid specifying the executor, because in some cases it even causes segfaults and the like (stopping criteria), but even then, avoiding the aforementioned penalties would require special-case code in each class made from a factory, instead of cleanly separating the responsibility of cross-executor copies into a single-purpose class.
  • The same logic also goes for precision conversion boundaries: While having the temporary conversion makes sense for some cases, if you need good performance, I would like to provide a separate precision conversion class that stores the temporaries explicitly.

The suggested approach is a natural extension of the Reordered, ScaledReordered, ... design into other Ginkgo functionality (precisions and executors)

@yhmtsai
Copy link
Member Author

yhmtsai commented Oct 5, 2022

Some algorithms only work on the cpu then they will convert twice in apply and need to take care the data location in implementation if we always force them into gpu.
Creating two executors and using different executor on different object is more hard than using one executor for everything.
When user create two executors, they know it may give some penalty on copy or something else.
I feel it is a conflict design against the executors - we provide several kinds of executor then we only allow using one of them.

@MarcelKoch
Copy link
Member

closed due to conceptual changes. Now we actually enforce that objects are copied to the right executor.

@MarcelKoch MarcelKoch closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod:core This is related to the core module. type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants