Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@swernli
Copy link
Collaborator

@swernli swernli commented Mar 7, 2021

This change introduces a refactor and fixup of the CMake infrastructure and powershell scripts for our QirRuntime build. This is part of the work needed to address #544. In particular, it splits out what was one dynamic library target, qdk, and turns it into three targets for the main QIR runtime, the QSharp Core functionality (intrinsic gates and simulator), and the QIR tracer. It fixes the generation of that dll by including additional support for dllexport when compiling on Windows and updates the corresponding IR files to also properly export API. The test for dynamic loading was subtly incorrect in a way that resulted in actually statically linking the dependencies, so this adjusts the test CMake to more explicitly use dynamic linking.

Renaming any variables and refactoring tests further into their own separate CMake project were out of scope for this change and will be addressed in a later PR.

@swernli swernli force-pushed the swernli/qirruntime-refactor branch from 4576d00 to 2ed8c91 Compare March 8, 2021 01:26
@swernli swernli requested a review from kuzminrobin March 8, 2021 23:21
@swernli swernli marked this pull request as ready for review March 8, 2021 23:23
@swernli swernli requested review from bettinaheim and cesarzc March 8, 2021 23:24
@swernli swernli requested a review from bamarsha March 8, 2021 23:24
@swernli
Copy link
Collaborator Author

swernli commented Mar 8, 2021

Of note: we want to pick better names than qir.dll, QSharpCore.dll and qirtracer.dll, ideally something that matches our naming schema of Microsoft.Quantum.*. Given that these don't have any specific project file or corresponding namespace like the C# code there isn't an obvious choice. Any suggestions? So far my best thought is: Microsoft.Quantum.Qir.Runtime.dll, Microsoft.Quantum.Qir.Qsharp.Core.dll, and Microsoft.Quantum.Qir.Tracer.dll.

@ScottCarda-MS
Copy link
Contributor

Of note: we want to pick better names than qir.dll, QSharpCore.dll and qirtracer.dll, ideally something that matches our naming schema of Microsoft.Quantum.*. Given that these don't have any specific project file or corresponding namespace like the C# code there isn't an obvious choice. Any suggestions? So far my best thought is: Microsoft.Quantum.Qir.Runtime.dll, Microsoft.Quantum.Qir.Qsharp.Core.dll, and Microsoft.Quantum.Qir.Tracer.dll.

Microsoft.Quantum. makes sense because that's who we are. Qir makes sense because that is the more specific area. Runtime.dll Core.dll and Tracer.dll make sense because those are the names of the specific dlls. What does the Qsharp part of the Microsoft.Quantum.Qir.Qsharp.Core.dll infer? Are there multiple dlls under the idea of Qsharp? It might me a bit redundant because Qir is already specific to Q#, right? Or perhaps it is the case that the Runtime.dll and the Tracer.dll are agnostic of the QIR coming from Q#, whereas the Core.dll is specific to QIR that was generated from Q#? I could see the necessity for Qsharp in the name if that were the case. Otherwise I'm leaning towards dropping the Qsharp from core's name. Otherwise the naming you suggest makes perfect sense to me, and I like it. That's my 2 cents anyways.

@swernli
Copy link
Collaborator Author

swernli commented Mar 9, 2021

Yeah, I agree that Microsoft.Quantum.Qir.QSharp.Core.dll is both the wordiest and the hardest to reason about. A few points to respond to...

Are there multiple dlls under the idea of Qsharp?

No, I don't expect there will be more under QSharp which makes it weird to have Core under there. I feel like our names end up trying to serve dual purposes, where the dots form a hierarchy like a folder structure, while also forming the logical separation in a descriptive name. The meaning I was aiming for is that this dll is the QIR support for the QSharp Core package, which is the default target package when compiling Q#. This is as opposed to Type1.Core or Type2.Core which are target packages with alternate gate sets and would require a different QIR dll to compile to an executable. We already have Microsoft.Quantum.QSharp.Core.dll which comes from this repo, under src/Simulation/QSharpCore. Since everything defined in here is under the __quantum__qis prefix because it corresponds to Q# callables defined as body intrinsic, maybe Microsoft.Quantum.Qir.Intrinsic.QSharp?

It might me a bit redundant because Qir is already specific to Q#, right?

QIR itself is not Q# specific, which is an important point worth doubling down on. Alan's blog post announcing QIR put it very directly:

QIR is intended to serve as a common interface between many languages and target quantum computation platforms. Thus, while it supports Q#, QIR is not specific to Q#: any programming language for gate-based quantum computing can be represented in QIR. Similarly, QIR is hardware-agnostic: it does not specify a quantum instruction or gate set, leaving that to the target computing environment.

That idea was part of what motivated me to make this split into multiple dlls in the first place. Having a set of gates defined in the QIR runtime, even if it's intended as an optional interface developers can choose not to inherit from, implies tying the infrastructure to a specific quantum hardware. So the idea was to separate the quantum gates from the rest and make it easy for someone to swap in a different set of intrinsic gates. That said, I'm realizing on further review that there is still some mixing going on here, because while everything defined in the runtime (prefixed with __quantum__rt) matches the QIR spec and is meant to be generic support that can work on any platform or target, we still have a significant number of intrinsic elements (prefixed with __quantum__qis) in the runtime dll that aren't quantum gates. Things like the math functions which are generally useful but not really "quantum". I'm wondering if it's worth splitting those out as well, since that piece is specific to the Q# language?

Edit: Removed some misunderstandings regarding which parts are Q# specific.

Copy link
Contributor

@kuzminrobin kuzminrobin left a comment

Choose a reason for hiding this comment

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

Impressive number of changes.

@swernli
Copy link
Collaborator Author

swernli commented Mar 9, 2021

Just thinking out loud here, but I wonder if it makes sense to split this out even further, perhaps into another repo (yes, I know the last thing we want is another repo, but hear me out). Everything that is part of __quantum__qis is part of a specific target's quantum intrinsics. In our case, these come from body intrinsic Q# callables defined in either Microsoft.Quantum.QSharp.Foundation as part of the common language support or are part of a target package defining a quantum gate set like like Microsoft.Quantum.QSharp.Core. So maybe it makes sense to have all the truly generic elements in __quantum__rt part of their own repo along with the QIR spec that produces the library that all QIR consumers or developers would need, and then in this repo having only the Q# specific portions that are part of the qis.

@bettinaheim, @alan-geller Any thoughts on this?

@@ -0,0 +1 @@
Checks: '-*,bugprone-*' No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering what were the specific reasons for not using the parent's clang-tidy configuration?

One pattern that might be useful if it is just a couple of checks that are different from the parent configuration would be using InheritParentConfig: true and adding/removing the necessary checks.

Copy link
Contributor

@cesarzc cesarzc left a comment

Choose a reason for hiding this comment

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

This change is great! I am a fan of the new naming convention and the ability to do dynamic linking :)

@swernli swernli merged commit c832ef3 into main Mar 10, 2021
@swernli swernli deleted the swernli/qirruntime-refactor branch March 10, 2021 15:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants