QIR emission throws if no explicit return ..; despite that all parts of if .. else .. do return. #877
Description
Discussion
[12:59 PM] Robin Kuzmin
Stefan,
... if I replace Round() with my own implementation like below, the 0.15.2102129370-alpha\tools\qsc\Microsoft.Quantum.QsCompiler.dll still throws an exception. My implementation:
function MyRound(value : Double) : Int { // 4.x 4.5 5.x 5.5 -4.x -4.5 -5.x -5.5
let truncated = Truncate(value); // 4 4 5 5 -4 -4 -5 -5
if truncated >= 0 {
let diff = value - IntAsDouble(truncated); // 0.x 0.5 0.x 0.5 diff
if diff < 0.5 { return truncated; } // 4 5 return
if diff > 0.5 { return (truncated + 1); } // 5 6 return
if truncated % 2 == 0 { return truncated; } // 4 return
else { return truncated + 1; } // 6 return
}
else {
let diff = IntAsDouble(truncated) - value; // 0.x 0.5 0.x 0.5 diff
if diff < 0.5 { return truncated; } // -4 -5
if diff > 0.5 { return (truncated - 1); } // -5 -6
if truncated % 2 == 0 { return truncated; } // -4
else { return truncated - 1; } // -6
}
}
Edited[12:59 PM] Robin Kuzmin
This is just for you to think about. It is not blocking me.
[1:01 PM] Bettina Heim
I did not change anything in the if-statement handling with the fix for conditional expressions (the two are entirely unrelated). What is the exception exactly?
[1:05 PM] Robin Kuzmin
The same logic implemented like below does not cause the excpetion (compiles successfully):
function MyRound(value : Double) : Int {
let truncated = Truncate(value); // 4 4 5 5 -4 -4 -5 -5
if truncated >= 0 {
let diff = value - IntAsDouble(truncated); // 0.x 0.5 0.x 0.5 diff
if diff < 0.5 { return truncated; } // 4 5 return
if diff > 0.5 { return (truncated + 1); } // 5 6 return
if truncated % 2 == 0 { return truncated; } // 4 return
return truncated + 1; // 6 return
}
let diff2 = IntAsDouble(truncated) - value; // 0.x 0.5 0.x 0.5 diff
if diff2 < 0.5 { return truncated; } // -4 -5
if diff2 > 0.5 { return (truncated - 1); } // -5 -6
if truncated % 2 == 0 { return truncated; } // -4
return truncated - 1; // -6
}
The exception looks the same as I reported recently. Just in case I provide again (to make sure it is the same or is different)
C:\ed\dev\QSharpCompiler\qsharp-runtime\qsharp-runtime\src\QirRuntime>python build.py debug
12:55:15: generating QIR for: \
C:\ed\dev\QSharpCompiler\qsharp-runtime\qsharp-runtime\src\QirRuntime\test\QIR-static
12:55:15: running: dotnet build \
C:\ed\dev\QSharpCompiler\qsharp-runtime\qsharp-runtime\src\QirRuntime\test\QIR-static\qsharp
Microsoft (R) Build Engine version 16.8.3+39993bd9d for .NET
Copyright (C) Microsoft Corporation. All rights reserved.
Determining projects to restore...
All projects are up-to-date for restore.
EXEC : error QS7109: The compilation step "QIR Generation" loaded from \
"C:\Users\rokuzmin\.nuget\packages\microsoft.quantum.sdk\0.15.2102129370-alpha\tools\qsc\
\Microsoft.Quantum.QsCompiler.dll" threw an exception. \
[C:\ed\dev\QSharpCompiler\qsharp-runtime\qsharp-runtime\src\QirRuntime\test\QIR-static\
\qsharp\qir-gen.csproj]
____________________________________________
Q# compilation failed: 1 error, 0 warnings
1 logged exception
[1:05 PM] Stefan Wernli
Huh... I was able to repro as well, and here is the exception:
System.ArgumentException: Return instruction for non-void function must have a value
at Ubiquity.NET.Llvm.Instructions.InstructionBuilder.Return() in D:\a\1\s\submodules\qsharp-compiler\src\QsCompiler\Llvm.NET\Instructions\InstructionBuilder.cs:line 277
at Microsoft.Quantum.QsCompiler.QIR.GenerationContext.EndFunction() in D:\a\1\s\submodules\qsharp-compiler\src\QsCompiler\QirGeneration\Context.cs:line 878
[1:05 PM] Stefan Wernli
it's mad about the fact that there isn't a typed value to return from outside of the if-statements
[1:07 PM] Stefan Wernli
a minimal test case would be:
function ReturnsInteger(negative : bool) : Int {
if negative { return -1; }
else { return 1; }
}
[1:08 PM] Robin Kuzmin
That was my suspicion also. The early C/C++ compilers also didn't like that all the branches of if {.. } else {.. } have return ..; , but there was no explicit return .. outside of if {..} else { .. } before returning from a func.
[1:08 PM] Bettina Heim
Hm, ok. The only thing that is confusing is why it worked before - I don't remember changing anything that could have impacted that. However, this is also all work that is still on a working branch of mine, so it is perfectly possible that I just forgot to clean something up. Unless this is blocking anyone, I'll make sure to add that test case when I get the changes ready for merging.
[1:09 PM] Stefan Wernli
no, it didn't work before. This implementation is a new function Robin wrote that exposed the error
[1:09 PM] Robin Kuzmin
I'm not blocked at all. Take your time.
(1 liked)[1:09 PM] Bettina Heim
Ah, that makes sense then. Ok, llvm is just more picky than I thought.
[1:10 PM] Stefan Wernli
right. LLVM throws if a void return is used in a function that is typed.
[1:11 PM] Stefan Wernli
so far, this wont cause any problems with ongoing work. But it would need to be fixed to avoid hitting this with any standard library or user-authored code
(1 liked)[1:11 PM] Bettina Heim
That's fair. Sounds like I have a spare return that shouldn't be there. As long as it is fine with not having a return after the ifs it shouldn't be too painful to fix.
(1 liked)
[1:17 PM] Robin Kuzmin
.. have a spare return that shouldn't be there if both if { .. } and else { .. } (and all nested ifs and elses) return, i.e. the execution never reaches after if { .. } else { .. }.
[1:19 PM] Stefan Wernli
The nice part is that the validation of all control paths returning a valid value is done by other parts of the Q# compiler, so QirGeneration just needs to skip adding the void return if the function is typed. I think I can fix this one and add an appropriate test.
(1 liked)[1:19 PM] Bettina Heim
Yes, the Q# compiler upon syntax tree validation enforces that all paths return a value unless the return value is of type unit. Where the awkwardness comes in upon QIR emission (and the reason for the bug) is that while Q# doesn't require an explicit return if the callable return Unit, I think llvm does (or maybe I just mistakenly assumed that?)
[1:19 PM] Bettina Heim
He he, another race condition in the reply. (smile)
[1:23 PM] Robin Kuzmin
I, as a human, typically rewrite the offending code with the following in mind:
If if { .. } part (and all its nested parts) returns, then I remove the subsequent else keyword (but leave the scope after else). I.e. I convert the code like this:
function ReturnsInteger(negative : bool) : Int {
if negative { return -1; }
else { return 1; }
}
into a code like this:
function ReturnsInteger(negative : bool) : Int {
if negative { return -1; }
// else { // I remove this.
return 1;
// } // I remove this.
}
That's approximately what I would expect from the compiler too. But I don't know how difficult it is to implement such a conversion in the compiler.
[1:26 PM] Robin Kuzmin
What might be easy to implement is the same what the early C/C++ compilers did. They just complained (for the offending code) that the function does not return a value by the final }.
Edited[1:29 PM] Stefan Wernli
We could do that, but it would be a language change. Right now, Q# is smart enough to see that if all branches of an if-statement return then the whole function returns. The QIR fix is easy enough to implement (I think), so the language can be left as-is.
(1 liked)[1:30 PM] Robin Kuzmin
Sounds good to me! (smile)
[2:41 PM] Robin Kuzmin
Bettina,
In my test I need to leave certain fragment commented out and removed after the Q# compiler issue is resolved. Do we have the issue number for my fragment to refer? (or shall I open an issue for that?)
[2:41 PM] Bettina Heim
Not yet. Please open an issue on the compiler repo. Thanks!