Compiler conformance: Quantum declaration #219
Conversation
| auto mlir = qcor::mlir_compile(complex_if, "complex_if", | ||
| qcor::OutputType::MLIR, false); | ||
| std::cout << mlir << "\n"; | ||
| // qcor::execute(complex_if, "complex_if"); |
There was a problem hiding this comment.
This commented line in quantum declaration test blows up memory because of the measurement assignment statement.
If the preceding testcase is commented this works. It could be gtest doesn't do a tear down after each test?
moar55
changed the title
moar55
changed the title
There was a problem hiding this comment.
Summary comments:
(1) we need to fix these diffs - a good number of the files have global changes to the whole file which makes it difficult to see what changed in the file. May need to reclone and copy changes in and re-PR.
(2) All changes to adhere to latest from upstream (at least in this case) should be additions and not modifications breaking backwards compatibility. Tests should not be updated to reflect the new syntax, but instead, you should add a new test to demonstrate and test the new syntax.
| add_test(NAME qcor_qasm3_test_nisq_ifstmt COMMAND qasm3CompilerTester_nisq_ifstmt) | ||
| target_include_directories(qasm3CompilerTester_nisq_ifstmt PRIVATE . ../../ ${CMAKE_SOURCE_DIR}/tools/clang-wrapper/ ${CMAKE_SOURCE_DIR}/handlers ${XACC_ROOT}/include/gtest) | ||
| target_link_libraries(qasm3CompilerTester_nisq_ifstmt qcor-mlir-api gtest gtest_main qcor-clang-wrapper qcor-syntax-handler ${CLANG_LIBS}) | ||
|
|
There was a problem hiding this comment.
I'm seeing a bunch of these diff files where you've deleted the entire file and replaced with new content. This makes it hard to see what you've changed. Can you try to fix this? You may need a fresh fork and copy just your changes into that fork, then issue a new PR.
There was a problem hiding this comment.
Not sure why that happened but I will fix it in a new PR 👍
| @@ -4,7 +4,7 @@ | |||
| TEST(qasm3VisitorTester, checkAlias) { | |||
| const std::string alias_by_indicies = R"#(OPENQASM 3; | |||
| include "qelib1.inc"; | |||
| qubit q[6]; | |||
| qubit[6] q; | |||
There was a problem hiding this comment.
One thing to note - I want this implementation to conform to the qasm3.g4 spec upstream, but we are free to keep existing, or introduce novel, syntax. So in this case, yes, we should update the parser to handle qubit[N] q, but we should make sure that the existing qubit q[N] also works for our own backwards compatibility.
| @@ -486,7 +486,7 @@ include "qelib1.inc"; | |||
| // phi_est = 11/16 (denom = 16 since we have 4 bits) | |||
| // => phi = 2pi * 11/16 = 11pi/8 = 2pi - 5pi/8 | |||
| // i.e. we estimate the -5*pi/8 angle... | |||
| qubit q[2]; | |||
| qubit[2] q; | |||
There was a problem hiding this comment.
Going back to the comment below - instead of changing all occurrences of qubit declarations to be in the new syntax, I prefer you keep it all as is, and add a new test that shows the compiler can handle this new syntax. I'm really interested in conserving backwards compatibility with what we already have, while at the same time ensuring we are compatible with these latest changes from upstream.
| @@ -1,485 +1,499 @@ | |||
| #include "gtest/gtest.h" | |||
There was a problem hiding this comment.
This is another case of deleting the whole file and replacing with new contents, which has messed up the diff.
|
Would such syntax be also valid: |
|
Good question. My initial thought is no. You should be consistent in single line comma separated declarations. In the case you specify, I think qubit[4] implies a specific type (namely an array of 4 qubits) and that should be for all variables in the comma separated declaration. |
|
sounds good 👍 |
This pull request conforms QASM3-MLIR conversions of quantum declarations with the latest QASM3 grammar. Relevant to #190