-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allow construction from module and specific entry point name #5
Conversation
c50dfc4
to
67a1139
Compare
454b207
to
079e397
Compare
079e397
to
c5397d3
Compare
This is useful when a user generates an llvm::Module that may have multiple entry points, as the Qwerty compiler/runtime does.
In my experience, it has been easier to treat XaccQuantum as a singleton to avoid issues with XACC initializing itself twice. It is probably cleaner to pass (accelerator, shots) via qiree::Executor, but this solves my immediate implementaiton problem in the Qwerty compiler/runtime.
c5397d3
to
3a6106f
Compare
src/qirxacc/XaccQuantum.cc
Outdated
/*! | ||
* Update the XACC accelerator and shot count. | ||
*/ | ||
void XaccQuantum::set_accelerator_and_shots( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done at initialization time (which the constructor does) rather than at some arbitrary point after construction (where it could be in an unspecified state, like in the middle of a calculation). There shouldn't be any need for delayed partial initialization (bad practice in C++); can you explain the use case?
Also please remember to run clang-format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because XaccQuantum
effectively needs to be used as a singleton right now because xacc::Finalize()
does not clean up the state of XACC properly, meaning you cannot currently create two consecutive XaccQuantum
instances.
Details (if you're interested)
To see this in action, you can add this:$ git diff
diff --git a/app/qir-xacc.cc b/app/qir-xacc.cc
index 383fb6f..2bb6156 100644
--- a/app/qir-xacc.cc
+++ b/app/qir-xacc.cc
@@ -36,6 +36,11 @@ void run(std::string const& filename,
// Load the input
Executor execute{Module{filename}};
+ {
+ XaccQuantum xacc_temp(std::cout, accel_name, num_shots);
+ (void)xacc_temp;
+ }
+
// Set up XACC
XaccQuantum xacc(std::cout, accel_name, num_shots);
std::unique_ptr<RuntimeInterface> rt;
Then, when running a trivial example:
$ gdb --args qir-xacc -i bell.ll -a qpp
[snip]
terminate called after throwing an instance of 'std::runtime_error'
what(): The bundle context is no longer valid
[snip]
(gdb) bt
[snip]
#10 0x00007ffff7aae2dd in cppmicroservices::BundleContext::GetServiceReferences<xacc::OptionsProvider> (this=0x55555856b208, filter="")
at /home/austin/Documents/school/gatech/grad/xacc/xacc/tpls/cppmicroservices/framework/include/cppmicroservices/BundleContext.h:445
#11 0x00007ffff7aabe65 in xacc::ServiceRegistry::getServices<xacc::OptionsProvider> (this=0x55555856b1e0)
at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/service/ServiceRegistry.hpp:161
#12 0x00007ffff7aa741d in xacc::ServiceRegistry::getRegisteredOptions[abi:cxx11]() (this=0x55555856b1e0)
at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/service/ServiceRegistry.hpp:187
#13 0x00007ffff7aa6268 in xacc::getRegisteredOptions[abi:cxx11]() ()
at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/service/xacc_service.cpp:74
#14 0x00007ffff7ae41b8 in xacc::CLIParser::parse (this=0x7ffff00ef4c0, argc=1, argv=0x5555585aa7c0)
at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/utils/CLIParser.cpp:48
#15 0x00007ffff7b5a718 in xacc::Initialize (arc=1, arv=0x5555585aa7c0) at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/xacc.cpp:92
#16 0x00007ffff7b5a2ab in xacc::Initialize (argv=std::vector of length 1, capacity 1 = {...})
at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/xacc.cpp:60
#17 0x00007ffff7b5a329 in xacc::Initialize () at /home/austin/Documents/school/gatech/grad/xacc/xacc/xacc/xacc.cpp:63
#18 0x0000555555849dd8 in qiree::XaccQuantum::XaccQuantum (this=0x7fffffffd120, os=..., accel_name="qpp", shots=1024, __in_chrg=<optimized out>,
__vtt_parm=<optimized out>) at /home/austin/Documents/school/gatech/grad/qsvt/qiree/src/qirxacc/XaccQuantum.cc:46
#19 0x00005555557f339f in qiree::app::run (filename="bell.ll", accel_name="qpp", num_shots=1024, print_accelbuf=true, group_tuples=false)
at /home/austin/Documents/school/gatech/grad/qsvt/qiree/app/qir-xacc.cc:45
#20 0x00005555557f3a27 in main (argc=5, argv=0x7fffffffd9b8) at /home/austin/Documents/school/gatech/grad/qsvt/qiree/app/qir-xacc.cc:98
I just opened this XACC PR to fix this problem for the sake of future QIR-EE users: ORNL-QCI/xacc#63. I removed this setter from this PR, but I will keep it in my fork because I do not want to re-do xacc::Initialize()
and xacc::Finalize()
several times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, makes sense since xacc has singletons. I'd prefer that we hide that kind of bad behavior in our implementation: for example, we could make a private helper class that cleanly manages XACC cleanup/teardown and would be called in the constructor/destructor. It's probably not a bad idea to do that anyway, since we've got this object oriented facade built on top of a singleton class: creating two different XaccQuantum classes simultaneously would wreak all sorts of havoc.
/*! | ||
* Construct with an LLVM module and an entry point. | ||
*/ | ||
Module::Module(UPModule&& module, std::string const& entrypoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't QIR require a function to be specified as an entry point? or is this for the case where you have more than one entry point in a QIR file (and is that legal?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or is this for the case where you have more than one entry point in a QIR file
Exactly, yeah. Sometimes the Qwerty runtime can produce a QIR module with multiple entry points.
(and is that legal?)
I couldn't anywhere that the spec says that multiple entry points are allowed, but it doesn't seem to say that only one entry point is allowed either.
I learned this was possible from reading the qir-runner source code: 1, 2. The tl;dr is that if there is one entry point, qir-runner uses it, but otherwise they require specifying the name of the entry point to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's already a similar constructor overload that takes an entrypoint name, just with a filename instead of an llvm::Module
:
Line 129 in 9c1ff4c
Module::Module(std::string const& filename, std::string const& entrypoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and I forgot about that function! I wonder if we need to restructure so that the Executor::operator()
takes the entry point function name since one module has to be correspond exactly to a single executor, and if we have multiple entry points in one module it would be better not to have to recreate the module each time?
c62e712
to
1626b8e
Compare
These are useful for me downstream in the Qwerty compiler/runtime.
It would be nicer to pass the accelerator and shots as arguments to
qiree::Executor
somehow, but this solves my immediate engineering problem.Testing
Once rebased against #4, this builds and passes
ctest
. I previously tested this code end-to-end in the Qwerty runtime but the code is in a state where it will be tough to do that again.