[WIP] Prevent side-effect binding elimination

[ncave]

In Optimized AST, prevents binding elimination of pattern match tests with side-effects.
(i.e. prevents multiple executions of the same pattern match test with side-effects).

[ncave]

Almost everything is passing, except fsharpqa:

3199 tests passed (99.94%)
2 tests failed (0.06%)
0 tests were cascaded failures (0.00%)
0 tests returned "no result" (0.00%)
0 tests timed out (0.00%)
0 tests had test errors (0.00%)
========
3201 Total executed tests

Perhaps the IL for those 2 tests can be updated to match the new behavior (un-eliminated binding).

[KevinRansom]

This feels like a good change.

I can't help thinking that some working, but buggy code is now going to work differently following a recompile with an updated compiler. However, I am in favour of developers fixing their code.

[ncave]

Rebased to latest.

[KevinRansom]

Including us, by the looks:

CodeGen\EmittedIL\TestFunctions (TestFunction9b4.fs) -- failed
CodeGen\EmittedIL\QueryExpressionStepping (Linq101Aggregates01.fs - CodeGen) -- failed

[ncave]

@KevinRansom Yes, that's what is mentioned in my second comment, see above.
IMO these 2 tests just need to be updated to reflect the new (arguably safer, i.e. more conservative) behavior.

[TIHan]

I'm not sure of the change. Binding elimination of x for let x = e in match x with ... is a safe optimization regardless if e causes a side effect or not.

What is your reasoning for the change? You said:

prevents multiple executions of the same pattern match test with side-effects

I'm not quite sure I understand this. Can you expand this explanation with an example?

[ncave]

@TIHan The test included in the PR provides an example.
The let binding in the match test gets eliminated and converted into multiple test executions in the if-then chain:

match side_effect() with

will be converted to (in pseudo-code):

if side_effect() = x then ...
else if side_effect() = y then ...
else ...

[TIHan]

@ncave Thanks, that gives me clarity, specifically your pseudo-code. It's great that you have identified this.

In principle, I think the optimized AST should not have a representation of multiple executions from the match optimization of what you described:

if side_effect() then ...
else if side_effect() then ...
else ...

This doesn't feel right for both side effect and side effect free executions, not just side effects.

However, the actual IL generated doesn't have the multiple executions; somewhere down the line it gets re-written or IL generated in a special way. With your change, the IL gets generated differently; the binding elimination doesn't happen and it creates a local. Therefore, this hurts an existing optimization for us.

My assumption is that this is for Fable; you all are relying on the optimized AST and you all hit this. Is that correct?

[ncave]

@TIHan Yes, this issue (as stated in the PR) is manifesting in Optimized AST only.
The argument the PR is making is that this optimization is (arguably) perhaps insufficiently constrained.
With this change, the optimization still happens, just less often (more conservatively).

I'm unaware what other clients (besides Fable) are a consumer of optimized F# AST.

[dsyme]

I agree with @TIHan so far, the optimization as implemented to this seems to be sound.

So I'm not understanding which Fable problem this is fixing - @ncave could you add an issue for this? I assume it is different to the issue you fixed here #6333

[dsyme]

needs a matching issue

[ncave]

@dsyme Yes, it's unrelated to #6333.
I have included a test in the PR that showcases the issue.

In plain words, the match test binding elimination optimization is incorrect when there are multiple cases (more than 2) to match, and the match test has side effects.

But it's possible it may be a bit strong in its current form, perhaps we need to only check for side effects if more than 1 replacement is made. I'll see if I can fix that.

[dsyme]

@ncave Yes, I understand that this must fix an issue in the optimized expression trees being seen via the FCS API. However it seems to affect generated code too, by disabling the application of the optimization when it is sound.

Two questions

• What are those examples giving at the moment in the FCS API?
• Is there an example that shows the optimization unsound in regular F# code - i.e. apart from its reveal through the FCS optimized expressions API? (It could be the mistake is in the preparation of TASTs for the FCS API, or there is another interaction going on)

Either way I really want to dig into what's up here, understand it, make sure we have full info on the issue and . THanks :)

[ncave]

Keeping it downstream as not many clients use optimized AST.