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

Conversation

@kuzminrobin
Copy link
Contributor

@kuzminrobin kuzminrobin commented Feb 5, 2021

The first step for #501
Resolves #508

@kuzminrobin kuzminrobin force-pushed the kuzminrobin/mathfuncs branch from 47e5a73 to 0cf4a5e Compare February 5, 2021 02:50
Copy link
Contributor

@cgranade cgranade left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, but I'm not an LLVM expert per se so I'll leave it to others to approve. In any case, it looks like the tests are all at the C level, but it would be good to have integration tests similar to

define i64 @Microsoft__Quantum__Testing__QIR__QuantumRandomNumberGenerator__body() #0 {
that ensure that the new intrinsic works correctly when called from QIR files generated by the Q# compiler.

@kuzminrobin kuzminrobin force-pushed the kuzminrobin/mathfuncs branch from 0cf4a5e to 1527b19 Compare February 6, 2021 02:57
@kuzminrobin
Copy link
Contributor Author

This looks reasonable to me, but I'm not an LLVM expert per se so I'll leave it to others to approve. In any case, it looks like the tests are all at the C level, but it would be good to have integration tests similar to

define i64 @Microsoft__Quantum__Testing__QIR__QuantumRandomNumberGenerator__body() #0 {

that ensure that the new intrinsic works correctly when called from QIR files generated by the Q# compiler.

Moved the tests to .qs file.

Copy link

@guenp guenp left a comment

Choose a reason for hiding this comment

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

Just some comments; thanks for implementing this!

@kuzminrobin kuzminrobin force-pushed the kuzminrobin/mathfuncs branch from 1527b19 to dc623c1 Compare February 6, 2021 22:52
@kuzminrobin kuzminrobin changed the title Test PR: Added sqrt implementation Added some math funcs implementation Feb 8, 2021
@kuzminrobin kuzminrobin marked this pull request as ready for review February 8, 2021 19:37
Copy link
Contributor

@IrinaYatsenko IrinaYatsenko left a comment

Choose a reason for hiding this comment

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

Looks good to me on the QirRuntime's side of things. Signing off modulo Chris' comments on the semantics of arctan2.

Copy link
Contributor

@cgranade cgranade left a comment

Choose a reason for hiding this comment

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

Approving to unblock @guenp, but we should as quickly as possible:

  • Ensure exclusive language is not used in codebase
  • Extend QSharpFoundation unit tests to cover what you noticed about ArcTan2, so that we can prevent regressions that would cause QIR and C# runtimes to disagree

@kuzminrobin
Copy link
Contributor Author

Approving to unblock @guenp, but we should as quickly as possible:

  • Ensure exclusive language is not used in codebase
  • Extend QSharpFoundation unit tests to cover what you noticed about ArcTan2, so that we can prevent regressions that would cause QIR and C# runtimes to disagree

Filed #508.
Added to #506:

Extend QSharpFoundation unit tests to cover what has been noticed about ArcTan2, so that we can prevent regressions that would cause QIR and C# runtimes to disagree

@kuzminrobin kuzminrobin merged commit fbd4b38 into main Feb 9, 2021
@kuzminrobin kuzminrobin deleted the kuzminrobin/mathfuncs branch February 9, 2021 04:34
@kuzminrobin kuzminrobin self-assigned this Feb 23, 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.

Replace the exclusive term Implement bridge and native backing for math functions Q# depends on

5 participants