Emit condition check at the end of loop

[kerams]

Fixes #12138.

The IL produced by F# and C# for the given snippet is now identical. Not sure what (if anything) this means for debug points.

Is there a way to automatically regenerate the offending baseline files?

[smoothdeveloper]

Is there a way to automatically regenerate the offending baseline files?

Not sure if you tried this: https://github.com/dotnet/fsharp/blob/main/DEVGUIDE.md#updating-baselines-in-tests

If you are able to run those tests locally, it will create the "actual" files that you can then rename over the "baseline" ones (that will then show up in the diff).

[kerams]

Hmm, weird that some of the updated baselines still fail.

[dsyme]

@kerams Did you update using release mode, e.g.

.\build -testFSharpQA -c Release

plus also the other test suites? Thanks. There can be a codegen difference

[dsyme]

We will need to check debugging experience manually.

• For loops and while loops in synchronous code
• For loops and while loops in task code
• For loops and while loops in list expressions and array expressions
• For loops and while loops in sequence expressions

This is one way on Windows

artifacts\bin\fsc\Debug\net472\fsc.exe --optimize- -tailcalls- tests\walkthroughs\DebugStepping\TheBigFileOfDebugStepping.fsx

devenv /debugexe TheBigFileOfDebugStepping.exe

and then find the above constructs, breakpoint on the function defining them, and check F11 stepping behaviour is as before.

[kerams]

@dsyme, thanks. the debug point needed moving in front of the condition. While loop stepping works now but I'm not 100% sure about foreach, so another set of eyes would be appreciated.

Additionally, what's going on with these tests? The build did generate new providedDesigner.dll for me, but the tests still fail locally, so I did not commit the files. The message comes from PEVerify. Does it indicate a flaw in my loop implementation?

[dsyme]

@kerams here; https://stackoverflow.com/questions/13274384/why-is-this-net-il-not-verifiable

I'd expect you're hitting this in situations where there is something already on the IL stack. There could be a repro in code like this:

let f() =
    callSomething firstArgument ((while .... do ...); secondArgument)

Here firstArgument will be pushed and then the while loop generated. This code is obviously not common (especially without the try/finally implied by a for x in ... loop which later generates a while loop).

It might be you have to either avoid doing the codegen when the IL stack is non-empty. Or else you have to save the stack and restore it, which we already do for integer for-loops https://github.com/dotnet/fsharp/pull/12959/files#diff-bff3f7337de1c9a878112e73f774cc564bcae3e1e4785c8b1beb2af6cb8f6de9L4026

[dsyme]

@dsyme, thanks. the debug point needed moving in front of the condition. While loop stepping works now but I'm not 100% sure about foreach, so another set of eyes would be appreciated.

@kerams Great. Let's make sure we have me or someone else do an end-to-end double check of the debug stepping in all the necessary cases

[kerams]

It might be you have to either avoid doing the codegen when the IL stack is non-empty. Or else you have to save the stack and restore it, which we already do for integer for-loops

That was it :). I've decided to go the simpler route and fall back to using the old implementation when the stack is not empty and I've added a test for that specifically. As you're saying, it's a bit of an edge case and I suspect saving/restoring the stack could well make the whole thing even slower than it is, which would defeat the purpose of the PR.

Ready for review, I think.

[vzarytovskii]

Will it be a lot of work to have two separate tests in FSharp.Compiler.ComponentTests\EmittedIL which will verify both branches, to see if we have expected codegen?
Thanks.

[kerams]

2 such tests are in the new WhileLoops.fs. It's in ComponentTests, not as a baseline file though.

[vzarytovskii]

2 such tests are in the new WhileLoops.fs

Oh, apologies, it seems that GH just collapsed the file and I missed it.
Thanks!

[dsyme]

Looks like this has unfortunately collided with moving some tests , could you fix things up @kerams ? Thanks

[kerams]

@dsyme, done.

[vzarytovskii]

This looks good to me, thanks @kerams!
Did you, by any chance, look at any JIT benchmarks, were there any changes?

[kerams]

I didn't, but since the IL is now aligned with C#, we should see results similar to what was reported in the original issue.

[dsyme]

@vzarytovskii Note we still need to double-check the debug experience here for the various cases I listed above #12959 (comment)

[vzarytovskii]

@vzarytovskii Note we still need to double-check the debug experience here for the various cases I listed above #12959 (comment)

Oh, I did go thru the TheBigFileOfDebugStepping before merging and verified for and while in synchronous code, task builder, list, array and seq expressions.

Sorry, forgot to mention it.