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

Conversation

@troels-im
Copy link
Contributor

@troels-im troels-im commented Aug 18, 2021

CTest Infrastructure, test coverage and documentation

This PR adds:

  • Configuration files for tests in CMake
  • A profile test framework that allows to test transformations
  • Name testing of __quantum_(qis|rt) patterns through the passes directory to ensure consistency with
  • Documentation generation using Doxygen and markdown to produce a unified documentation server

Dependencies

No dependencies.

Documentation

This PR improves the documentation, by generating API and developer documentation using Doxygen. This documentation is combined with custom written pages. The result is packaged up as a docker image that serves all documentation via nginx. To build and serve the documentation, run

make serve-docs

Updated style checking

The style checker now sanitises all QIR qis symbols to make the development more error prune:

% ./manage stylecheck --fix-issues
2021-08-19 09:18:21,090 - FormatChecker - ERROR - Invalid string format for: '"__quantum__rt__qubit_allocate_array ' in Passes/Source/Rules/Factory.cpp
2021-08-19 09:18:21,911 - FormatChecker - ERROR - Invalid instruction 's_adj' in Passes/docs/src/base-profile-transformations.md
2021-08-19 09:18:21,912 - FormatChecker - ERROR - Missing specialisation for quantum symbol: __quantum__qis__h in Passes/docs/src/DeveloperGuide/QuickStart.md

This is still an experimental feature and despite reporting errors, the stylechecker will not fail at this point. The main reason is that #1121 should be merged before activating the enforcement of this.

Test coverage reports

Additionally, this PR adds test coverage reports for the individual tests. These can be generated as follows:

./manage  coverage --output [output_dir]

which will store a coverage report for each test in the library.

@swernli swernli requested review from idavis and swernli August 18, 2021 17:17
Copy link
Contributor

@swernli swernli left a comment

Choose a reason for hiding this comment

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

I have a few questions posted, but one thing I think we need follow up on is whether adding the submodule for googletest is something that requires an update to our NOTICE.txt that advertises the licenses for any external dependencies the repo has. @idavis for advice on handling the submodule.


function (microsoft_add_library
library)
list(REMOVE_AT ARGV 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the first element removed from ARGV? I don't see it used again anywhere, but might be missing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point, this is mostly just book keeping on what was already consumed and not. The reason why it is there is that at a later point, we may choose to extend the command such that it becomes microsoft_add_library(name dep1 dep2 ... depN) and by removing the arguments which were already consumed the arg list is ready to do this. However, there is no decision to do one or the other thing atm., so I have just prepared it for it.


if(MICROSOFT_ENABLE_DYNAMIC_LOADING)
target_link_libraries(${library}
"$<$<PLATFORM_ID:Darwin>:-undefined dynamic_lookup>")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this PLATFORM_ID:Darwin something that works on all platforms or is it Mac OS specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is OS X specific. I am not sure whether it is needed for other platforms, but a quick Google suggests that these look substantially different for Linux (and possibly Windows).

function (microsoft_add_library_tests
library)
if (MICROSOFT_ENABLE_TESTS)
list(REMOVE_AT ARGV 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here again, I'm don't see why removing the first item from ARGV is needed.

define internal fastcc void @TeleportChain__DemonstrateTeleportationUsingPresharedEntanglement__body() unnamed_addr {
entry:
call void @__quantum__qis__h(%Qubit* null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, with the coming update to the newer beta package, these now show up with __body at the end of the names. See the changes to the old readme here: #1121.
It probably makes sense to merge this PR first, since it includes build fixes, and then update 1121 to include changes to these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up. Do we have a full list of all symbol names somewhere? If so, I might just scan the entire Passes directory for invalid names.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have that full list in one place, no, but it comes from the change made to the runtime repo. In #1121 I went ahead and did what you suggest: searched the whole Passes directory and double checked every use of a __quantum__qis__ function to make sure I either regenerated the corresponding file or manually updated the references. I can just do that again after this PR merges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NIce, thanks!

@swernli swernli mentioned this pull request Aug 18, 2021
@swernli
Copy link
Contributor

swernli commented Aug 18, 2021

Regarding the test dependency on an external submodule, would it be possible to just use Catch2 like the qsharp-runtime repo? See https://github.com/microsoft/qsharp-runtime/tree/main/src/Qir/Common/Externals#catch2 for where we describe the dependency on the library, and https://github.com/microsoft/qsharp-runtime/blob/main/src/Qir/Tests/QIR-dynamic/qir-driver.cpp as an example of how we use it.

@troels-im
Copy link
Contributor Author

Regarding the test dependency on an external submodule, would it be possible to just use Catch2 like the qsharp-runtime repo? See https://github.com/microsoft/qsharp-runtime/tree/main/src/Qir/Common/Externals#catch2 for where we use the library, and https://github.com/microsoft/qsharp-runtime/blob/main/src/Qir/Tests/QIR-dynamic/qir-driver.cpp as an example of how we use it.

What I agreed with @bettinaheim is that we follow the standard used by LLVM and try to align with them as closely as possible. That said, I am happy for us to use Catch2 instead of gmock if the consensus is that this is a better choice.

@swernli
Copy link
Contributor

swernli commented Aug 18, 2021

What I agreed with @bettinaheim is that we follow the standard used by LLVM and try to align with them as closely as possible. That said, I am happy for us to use Catch2 instead of gmock if the consensus is that this is a better choice.

Ah, that makes sense. I could see it going either way; aligning with the patterns and processes used by LLVM is a good idea, but bringing in a submodule comes with its own considerations while this is under the Microsoft org on GitHub (this would be the first submodule added to any of our repos, though likely not the last). Catch2 is an easy, one file dependency, but means that our CMakeLists and test files would look rather different than the ones from LLVM, which also brings overhead for developers. Unfortunately, there isn't an easy or quick answer here.

docker run --rm -i -t qir-passes-docs:latest sh

serve-docs: documentation
docker run -it --rm -p 8080:80 --name qir-documentation -t qir-passes-docs:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

We should gen the docs into the project/docs folder as markdown instead of a container

@troels-im troels-im mentioned this pull request Aug 30, 2021
@troels-im troels-im merged commit 2cf15d1 into microsoft:features/llvm-passes Aug 30, 2021
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.

3 participants