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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 18, 2024
Merged

Allow construction from module and specific entry point name #5

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce5db78
Add constructor for llvm::Module + entrypoint name
ausbin Jun 30, 2024
db806b1
XaccQuantum: Add setter for accelerator/shots
ausbin Jun 30, 2024
3a6106f
Address PR comments
ausbin Oct 8, 2024
1626b8e
Address PR comments
ausbin Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/qiree/Module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,22 @@ Module::Module(UPModule&& module) : module_{std::move(module)}
<< module_->getSourceFileName() << "'");
}

//---------------------------------------------------------------------------//
/*!
* Construct with an LLVM module and an entry point.
*/
Module::Module(UPModule&& module, std::string const& entrypoint)
Copy link
Member

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?)

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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:

qiree/src/qiree/Module.cc

Line 129 in 9c1ff4c

Module::Module(std::string const& filename, std::string const& entrypoint)

Copy link
Member

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?

: module_{std::move(module)}
{
QIREE_EXPECT(module_);

// Search for explicitly named entry point
entrypoint_ = module_->getFunction(entrypoint);
QIREE_VALIDATE(entrypoint_,
<< "no entrypoint function '" << entrypoint << "' exists");
}


//---------------------------------------------------------------------------//
/*!
* Construct with an LLVM IR file (bitcode or disassembled).
Expand Down
3 changes: 3 additions & 0 deletions src/qiree/Module.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ class Module
// Construct from an externally created LLVM module
explicit Module(UPModule&& module);

// Construct from an externally created LLVM module and an entry point
Module(UPModule&& module, std::string const& entrypoint);

// Construct with an LLVM IR file (bitcode or disassembled)
explicit Module(std::string const& filename);

Expand Down
11 changes: 11 additions & 0 deletions src/qirxacc/XaccQuantum.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,17 @@ XaccQuantum::~XaccQuantum()
xacc::Finalize();
}

//---------------------------------------------------------------------------//
/*!
* Update the XACC accelerator and shot count.
*/
void XaccQuantum::set_accelerator_and_shots(
Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Member

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.

std::string const& accel_name, size_type shots) {
accelerator_ = xacc::getAccelerator(accel_name);
QIREE_VALIDATE(accelerator_, << "failed to create accelerator");
accelerator_->updateConfiguration({{"shots", static_cast<int>(shots)}});
}

//---------------------------------------------------------------------------//
/*!
* Prepare to build a quantum circuit for an entry point.
Expand Down
7 changes: 7 additions & 0 deletions src/qirxacc/XaccQuantum.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ class XaccQuantum final : virtual public QuantumNotImpl
size_type num_qubits() const { return num_qubits_; }
//!@}

//!@{
//! \name Mutators
// Update the XACC accelerator and shot count
void set_accelerator_and_shots(
std::string const& accel_name, size_type shots);
//!@}

//!@{
//! \name Quantum interface
// Prepare to build a quantum circuit for an entry point
Expand Down
Loading