Create Rust QIR Runtime Wrapper Library #878

idavis · 2021-11-19T18:09:37Z

(QDK Release: No changes visible to the users)

QIR Runtime build is now done through cmake instead of PowerShell configuration.
microsoft-quantum-qir-runtime-sys package compiles and sets up embedding/linkage
CC/CXX/RC env vars are no longer needed
Adds a basic runtime driver for nonadaptive jit execution.

* Moving QIR runtime build config into qir_cmake_include CMakeLists.

kuzminrobin · 2021-11-19T20:08:18Z

(Feel free to ignore, I don't know Rust and the background of this effort)

src/Qir/microsoft-quantum-qir-runtime-sys

The "sys" part seems to me not very informative (does not tell me the purpose or the structure/contents). I would consider a more informative name instead of "sys".
The name "microsoft-quantum-qir-runtime-sys" seems to me somewhat redundantly long name for a dir, where the part "microsoft-quantum-qir" does not add any info. However if you want to keep the dir name in sync with the package name, then that totally makes sense.

kuzminrobin · 2021-11-19T20:14:52Z

(Feel free to ignore on the same reasons as in the previous comment)
In the dir "src" there seem to be the files of different purpose.
Some files (or parts of the files) seem to be a part of the build infrastructure,
some files (or parts) seem to be a regular source code,
and some parts seem to be the tests.
Placing the regular source code and the tests for it in the same file (and the same dir) may be a common practice in Rust (I don't know), but as for the files that are a part of the build infra, I would consider separating/moving them to a different dir from sources and tests.

kuzminrobin · 2021-11-19T20:23:39Z

src/Qir/microsoft-quantum-qir-runtime-sys/build.rs

+    if cfg!(target_os = "windows") {
+        let path_to_runtime_src = "..\\Runtime";
+        compile_runtime_libraries(path_to_runtime_src)?;     
+    } else {
+        let path_to_runtime_src = "../Runtime";
+        compile_runtime_libraries(path_to_runtime_src)?;
+    }


(Nit)
Consider deduplicating as much as possible, such that only the differing part is placed in if .. else ... Something like this (pseudocode):

Suggested change

if cfg!(target_os = "windows") {

let path_to_runtime_src = "..\\Runtime";

compile_runtime_libraries(path_to_runtime_src)?;

} else {

let path_to_runtime_src = "../Runtime";

compile_runtime_libraries(path_to_runtime_src)?;

}

let path_separator = (cfg!(target_os = "windows") ? "\\" : "/"); // Either \ or /

let path_to_runtime_src = ".." + path_separator + "Runtime"; // "..\Runtime" or "../Runtime"

compile_runtime_libraries(path_to_runtime_src)?;

kuzminrobin · 2021-11-19T20:25:23Z

src/Qir/microsoft-quantum-qir-runtime-sys/build.rs

+fn compile_runtime_libraries(path_to_runtime_src: &str) -> Result<(), Box<dyn Error>> {
+    let mut config = Config::new(path_to_runtime_src);
+
+    if cfg!(target_os = "windows") {
+        config.static_crt(true);
+    }
+
+    set_compiler(&mut config)?;
+    set_profile(&mut config)?;
+
+    config.generator("Ninja");
+
+    let _ = config.build();
+    Ok(())
+}


(Nit. I don't know Rust. Talking in C/C++ terminology)
I have an impression that the function always returns Ok(()). If true, then doesn't it make sense to return void?

In Rust, expressions and functions always must return a value (by contrast with void, which is the absence of any value). Typically, a function with no meaningful return value will be marked as returning () (similar to Q#'s Unit or F#'s unit). Here, the return type is Result<(), Box<dyn Error>>, meaning it can either be Ok(()) or Err(t) for some t of type Box<dyn Error>.

All that leads up to that Ok(()) is the Rust equivalent to a void return type in C++.

As for why it seems like it always returns Ok(()), the ? operator at lines 34 and 35 cause early returns when either value is an Error. That's somewhat similar to exceptions, but has the benefit of being much more strongly typed, as it's not possible for a caller to ignore exceptions and as there's no use of exceptional control flow needed.

kuzminrobin · 2021-11-19T20:37:05Z

src/Qir/microsoft-quantum-qir-runtime-sys/build.rs

+    if cfg!(target_os = "linux") {
+        let mut c_cfg = cc::Build::new();
+        let clang_11 = which::which("clang-11")?;
+        c_cfg.compiler(clang_11);
+        config.init_c_cfg(c_cfg);
+
+        let mut cxx_cfg = cc::Build::new();
+        let clangpp_11 = which::which("clang++-11")?;
+        cxx_cfg.compiler(clangpp_11);
+        config.init_cxx_cfg(cxx_cfg);
+    } else if cfg!(target_os = "windows") {
+        let mut c_cfg = cc::Build::new();
+        let clang = which::which("clang.exe")?;
+        c_cfg.compiler(clang);
+        config.init_c_cfg(c_cfg);
+
+        let mut cxx_cfg = cc::Build::new();
+        let clangpp = which::which("clang++.exe")?;
+        cxx_cfg.compiler(clangpp);
+        config.init_cxx_cfg(cxx_cfg);
+    } else if cfg!(target_os = "macos") {
+        // Use macos default
+    } else {
+        panic!("Unsupported platform")
+    }


(Nit. . . .)
Consider dedulplicating (such that only the difference is in if .. else..), something like (Rust/C/C++ pseudocode)

Suggested change

if cfg!(target_os = "linux") {

let mut c_cfg = cc::Build::new();

let clang_11 = which::which("clang-11")?;

c_cfg.compiler(clang_11);

config.init_c_cfg(c_cfg);

let mut cxx_cfg = cc::Build::new();

let clangpp_11 = which::which("clang++-11")?;

cxx_cfg.compiler(clangpp_11);

config.init_cxx_cfg(cxx_cfg);

} else if cfg!(target_os = "windows") {

let mut c_cfg = cc::Build::new();

let clang = which::which("clang.exe")?;

c_cfg.compiler(clang);

config.init_c_cfg(c_cfg);

let mut cxx_cfg = cc::Build::new();

let clangpp = which::which("clang++.exe")?;

cxx_cfg.compiler(clangpp);

config.init_cxx_cfg(cxx_cfg);

} else if cfg!(target_os = "macos") {

// Use macos default

} else {

panic!("Unsupported platform")

}

if cfg!(target_os != "macos") {

if cfg!(target_os != "linux") && cfg!(target_os != "windows"){

panic!("Unsupported platform")

}

let clang = which::which(

cfg!(target_os = "linux") ? "clang-11" : "clang.exe")?;

let mut c_cfg = cc::Build::new();

c_cfg.compiler(clang);

config.init_c_cfg(c_cfg);

let clangpp = which::which(

cfg!(target_os = "linux") ? "clang++-11" : "clang++.exe")?;

let mut cxx_cfg = cc::Build::new();

cxx_cfg.compiler(clangpp);

config.init_cxx_cfg(cxx_cfg);

}

kuzminrobin · 2021-11-19T20:56:55Z

src/Qir/microsoft-quantum-qir-runtime-sys/src/foundation.rs

+#[cfg(target_os = "linux")]
+const FOUNDATION_BYTES: &'static [u8] = include_bytes!(concat!(
+    env!("OUT_DIR"),
+    "/build/lib/QSharpFoundation/libMicrosoft.Quantum.Qir.QSharp.Foundation.so"
+));
+
+#[cfg(target_os = "macos")]
+const FOUNDATION_BYTES: &'static [u8] = include_bytes!(concat!(
+    env!("OUT_DIR"),
+    "/build/lib/QSharpFoundation/libMicrosoft.Quantum.Qir.QSharp.Foundation.dylib"
+));
+
+#[cfg(target_os = "windows")]
+const FOUNDATION_BYTES: &'static [u8] = include_bytes!(concat!(
+    env!("OUT_DIR"),
+    "/build/lib/QSharpFoundation/Microsoft.Quantum.Qir.QSharp.Foundation.dll"
+));


Just FYI (I'm not saying whether it is good or bad):
I have seen a case where a single name for the dynamic library (written in Rust?) is used for all 3 platforms (in C#). See here, here, and here. Written by Chris.

If that is not applicable to Rust then is it possible to write like this? (deduplicated)

const FOUNDATION_BYTES: &'static [u8] = include_bytes!(concat!( env!("OUT_DIR"), #[cfg(target_os = "linux")] "/build/lib/QSharpFoundation/libMicrosoft.Quantum.Qir.QSharp.Foundation.so" #[cfg(target_os = "macos")] "/build/lib/QSharpFoundation/libMicrosoft.Quantum.Qir.QSharp.Foundation.dylib" #[cfg(target_os = "windows")] "/build/lib/QSharpFoundation/Microsoft.Quantum.Qir.QSharp.Foundation.dll" ));

To clarify, I did that due to requirements in our CI and packaging pipeline for .NET projects; I wouldn't recommend following my approach in this case.

cgranade · 2021-11-19T21:08:58Z

(Feel free to ignore, I don't know Rust and the background of this effort)

src/Qir/microsoft-quantum-qir-runtime-sys

The "sys" part seems to me not very informative (does not tell me the purpose or the structure/contents). I would consider a more informative name instead of "sys". The name "microsoft-quantum-qir-runtime-sys" seems to me somewhat redundantly long name for a dir, where the part "microsoft-quantum-qir" does not add any info. However if you want to keep the dir name in sync with the package name, then that totally makes sense.

The suffix -sys in crate names is somewhat of a Rust idiom, and carries with it the meaning that the crate acts to wrap a non-Rust library in a Rust package.

swernli and others added 14 commits October 20, 2021 15:27

Basic Runtime Driver implementation

8805dd3

Remove leftover comment

f552c3a

fix copyright comment

98026b7

Include memory header

6e289b5

Merge branch 'main' into swernli/basicruntimedriver

87363b7

Adding static lib generation

f1b3dae

Merge branch 'main' into swernli/basicruntimedriver

d486177

Removing static lib changes

69312ac

Moving QIR runtime build config into CMakeLists. (#863)

2c15e3c

* Moving QIR runtime build config into qir_cmake_include CMakeLists.

Adding foundation and runtime rust bundling

3bf0e47

Removing mut_static dep

533df83

Trying to get windows working.

d1ef7eb

Making LLVM dependency optional

0edd58f

using full path to clang in cmake calls

d955aec

idavis requested review from kuzminrobin and swernli November 19, 2021 18:09

Merge branch 'main' into iadavis/rust-runtime-sys

1b010bb

kuzminrobin reviewed Nov 19, 2021

View reviewed changes

kuzminrobin approved these changes Nov 19, 2021

View reviewed changes

Changing package dir due to root Cargo.toml introduction.

f4ee602

swernli assigned swernli and cgranade and unassigned cgranade and swernli Nov 22, 2021

idavis merged commit f4f2812 into main Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Create Rust QIR Runtime Wrapper Library #878

Create Rust QIR Runtime Wrapper Library #878

Uh oh!

idavis commented Nov 19, 2021 •

edited by kuzminrobin

Loading

Uh oh!

kuzminrobin commented Nov 19, 2021

Uh oh!

kuzminrobin commented Nov 19, 2021

Uh oh!

kuzminrobin Nov 19, 2021

Uh oh!

kuzminrobin Nov 19, 2021

Uh oh!

cgranade Nov 19, 2021

Uh oh!

kuzminrobin Nov 19, 2021

Uh oh!

kuzminrobin Nov 19, 2021

Uh oh!

cgranade Nov 19, 2021

Uh oh!

cgranade commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Create Rust QIR Runtime Wrapper Library #878

Create Rust QIR Runtime Wrapper Library #878

Uh oh!

Conversation

idavis commented Nov 19, 2021 • edited by kuzminrobin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuzminrobin commented Nov 19, 2021

Uh oh!

kuzminrobin commented Nov 19, 2021

Uh oh!

kuzminrobin Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

kuzminrobin Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

cgranade Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

kuzminrobin Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

kuzminrobin Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

cgranade Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

cgranade commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

idavis commented Nov 19, 2021 •

edited by kuzminrobin

Loading