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

New launch kernel for python #2706

Draft
wants to merge 75 commits into
base: main
Choose a base branch
from

Conversation

annagrin
Copy link
Collaborator

@annagrin annagrin commented Mar 6, 2025

Description

New launch kernel for python

bmhowe23 and others added 30 commits October 17, 2024 14:33
I, Ben Howe <[email protected]>, hereby add my Signed-off-by to this commit: 86681ef

Signed-off-by: Ben Howe <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
…gument-converter-all-functions

Signed-off-by: Anna Gringauze <[email protected]>
…gument-converter-all-functions

Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
…ate-synthesis-python

Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: Anna Gringauze <[email protected]>
@annagrin annagrin marked this pull request as draft March 6, 2025 23:28
auto callableNames = wrapper->callableNames;
private:
/// Creates new context without mlir initialization.
std::unique_ptr<MLIRContext> createContext() {
Copy link
Collaborator Author

@annagrin annagrin Mar 6, 2025

Choose a reason for hiding this comment

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

@amccaskey @schweitzpgi I found this way of creating a context instead of passing it as an argument in an ArgWrapper - do you think this makes sense? If yes, I can simplify the code a bit and do the same for the remote simulator.

Copy link
Collaborator

@amccaskey amccaskey Mar 7, 2025

Choose a reason for hiding this comment

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

from the test failure, looks like you'll need to add

  registerLLVMDialectTranslation(*context);

here. See runtime/common/RuntimeMLIR.cpp:90

Copy link
Collaborator

Choose a reason for hiding this comment

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

If Eric is ok with this then I think its a fine way to remove the need for passing ModuleOp+Context all the way through launching the kernel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we're trying to coerce the Module and related MLIRContext to stick around even though they are going out of scope someplace. After spending a bit of time reading the code, it's not clear at all still.

pyAltLaunchKernel is getting a raw mlir::ModuleOp passed to it, which it uses. extractQuakeCodeAndContext (in REST QPU) is constructing its own MLIRContext, which it appears is trying to be a singleton instance. It's not entirely clear how this singleton would be the same one the ModuleOp used. I'm also not clear why we need a singleton MLIRContext. Since Python is itself multithreaded, that seems like a bottleneck and/or potential problem.

If we created mlir::ModuleOp instances and saved them in std::shared_ptr, I believe pybind11 would autogenerate the "right" code for us such that Python's reference counting would work with C++ shared ptr reference counting so that the Module and its context don't get deconstructed along the way.

auto dynamicResult = cudaq::altLaunchKernel(
name.c_str(), thunk, reinterpret_cast<void *>(wrapper), size,
uReturnOffset);
auto dynamicResult = cudaq::streamlinedLaunchKernel(name.c_str(), args);
Copy link
Collaborator Author

@annagrin annagrin Mar 6, 2025

Choose a reason for hiding this comment

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

@amccaskey @schweitzpgi here is where I no longer pass the module to PyRemoteRESTQPU.cpp

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.

4 participants