Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Jun 17, 2021

This fixes the second problem in #11407 by making a general improvement to what happens when we eliminate user-define values in favour of compiler-generated values.

Note this builds on #11690 and when reviewing you can compare using https://github.com/dsyme/fsharp/compare/r2...dsyme:r3?expand=1

In release code when we have:

let f() =
    ...
    let someCompilerGeneratedThing = expr
    let someUserValue = someCompilerGeneratedThing 
    ...someUserValue...

we use ValValue information to eliminate someUserValue in favour of someCompilerGeneratedThing giving

let f() =
    ...
    let someCompilerGeneratedThing = expr
    ...someCompilerGeneratedThing ...

However the good user-facing name someUserValue has been thrown away and, for example, someCompilerGeneratedThing will not appear in the debugger since it is, well, compiler generated.

Instead when this elimination occurs we can forcibly adjust the name of someCompilerGeneratedThing to be someUserValue and mark it as not compiler generated. This is always valid, logically speaking (otherwise the ValValue information would not be valid) and will always improve the debugging experience for release code.

This fixes the second problem in #11407 which introduced more compiler-generated bindings. It may also improve other cases where we generated compiler-generated bindings as part of optimization. Some baselines may need updating as a result (though most our baselines relevant to debugging are for debug code, not optimized code)

@kerams
Copy link
Contributor

kerams commented Jun 17, 2021

Very nice. I'm sorry for generating extra work for you though :).

@dsyme
Copy link
Contributor Author

dsyme commented Jun 17, 2021

Very nice. I'm sorry for generating extra work for you though :).

It's ok. I've wanted to see us do optimizations like this for a long time - they really do matter in F# code - but always held off. It's great to have them addressed

I do need to be much more proactive about code reviewing however, I shouldn't have spotted that late.

@dsyme
Copy link
Contributor Author

dsyme commented Jun 17, 2021

One baseline needed updating, it helps validate that we are indeed getting better names in release code

@vzarytovskii
Copy link
Member

Some of the changes (in tests) may be conflicting when #11687 is merged and vice versa.

@dsyme dsyme mentioned this pull request Jun 17, 2021
@dsyme dsyme merged commit 01be658 into dotnet:main Jun 17, 2021
@kerams
Copy link
Contributor

kerams commented Jun 18, 2021

@dsyme, what other low hanging fruit like the tuple elimination is there? It would be great to have a meta issue like this for optimizations, which would track a list of (not too hard) ideas that anyone would be able to find, pick up and implement.

@smoothdeveloper
Copy link
Contributor

@kerams good point, you may make a "code generation & optimisation" meta issue, and update https://github.com/dotnet/fsharp/blob/main/.github/ISSUE_TEMPLATE/feature_request.md to point to it among the other ones.

Some RFC also have pending potential latent optimisations that could be tracked as "for contributors" issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants