[RFC FS-1014] Struct unions (single case) #1262

Merged
merged 15 commits into from Jun 25, 2016

Conversation

Projects
None yet
7 participants
@dsyme
Contributor

dsyme commented Jun 14, 2016

This PR implements support for F# RFC FS-1014 - struct unions, for single-case union types, e.g.

[<Struct>]
type U2 = U2 of int * int

Putting this PR up for CI and visibility

  • Finish RFC
  • Basic implementation
  • Testing for reflection on struct union types
  • Testing for quotations involving struct union types
  • Testing for pattern matching and nested pattern matching (involves taking address of struct union field)
  • Testing for cross-assembly cases
  • All tests passing

So next step is to write the RFC and clear the last few test glitches

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Jun 15, 2016

Contributor

This is fantastic.

Contributor

TIHan commented Jun 15, 2016

This is fantastic.

@dsyme dsyme closed this Jun 15, 2016

@dsyme dsyme reopened this Jun 15, 2016

dsyme added some commits Jun 15, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 16, 2016

Contributor

This is getting closer to completion. I've added a todo list above

Contributor

dsyme commented Jun 16, 2016

This is getting closer to completion. I've added a todo list above

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 16, 2016

Contributor

While looking over the code generated in some test cases, I noticed that the insertion of "copyOfStruct" values when taking the address of structs was always generating mutable locals. Mutable locals are less easy to eliminate in the optimizer. The locals can be immutable when the operation for which we're taking the address is NeverMutates. See cacc03f

By adding this optimization the generated code becomes copy-free, see functions g1-g4 in this test case 0e72beb.

Some of these issues are discussed in the RFC.

Contributor

dsyme commented Jun 16, 2016

While looking over the code generated in some test cases, I noticed that the insertion of "copyOfStruct" values when taking the address of structs was always generating mutable locals. Mutable locals are less easy to eliminate in the optimizer. The locals can be immutable when the operation for which we're taking the address is NeverMutates. See cacc03f

By adding this optimization the generated code becomes copy-free, see functions g1-g4 in this test case 0e72beb.

Some of these issues are discussed in the RFC.

@dsyme dsyme changed the title from WIP: struct unions prototype to [RFC FS-1014] Struct unions (single case) Jun 17, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 17, 2016

Contributor

This is now ready for detailed review and then merging

Contributor

dsyme commented Jun 17, 2016

This is now ready for detailed review and then merging

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan There are a couple of places in the code that I think are fixes for corner cases of struct records. I'll add some line comments - we should add relevant testing too

Contributor

dsyme commented Jun 17, 2016

@TIHan There are a couple of places in the code that I think are fixes for corner cases of struct records. I'll add some line comments - we should add relevant testing too

| false, true, 0 ->
Some (mkStaticRecdFieldGet (rfref, tinst, m))
| false, false, 1 ->
- Some (mkRecdFieldGet g (argExprs.[0], rfref, tinst, m))

This comment has been minimized.

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan - THis is one of the cases. The code is about the witness for the solution of a trait constraint when the solution is a record field getter. I suspect (based on looking at the code further above) that there are cases where the input here could already be a byref.

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan - THis is one of the cases. The code is about the witness for the solution of a trait constraint when the solution is a record field getter. I suspect (based on looking at the code further above) that there are cases where the input here could already be a byref.

@@ -2575,11 +2575,21 @@ let CodegenWitnessThatTypSupportsTraitConstraint tcVal g amap m (traitInfo:Trait
| true, true, 1 ->
Some (mkStaticRecdFieldSet (rfref, tinst, argExprs.[0], m))
| true, false, 2 ->
- Some (mkRecdFieldSet g (argExprs.[0], rfref, tinst, argExprs.[1], m))

This comment has been minimized.

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan This is another of the cases - it is when a trait constraint solves to be a record field setter

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan This is another of the cases - it is when a trait constraint solves to be a record field setter

if fieldNameUnbound rfld.Name && not rfld.IsZeroInit
- then Some(rfld.Name, mkRecdFieldGet cenv.g (oldve',tcref.MakeNestedRecdFieldRef rfld,tinst,m))
+ then Some(rfld.Name, mkRecdFieldGetViaExprAddr (oldveaddr,tcref.MakeNestedRecdFieldRef rfld,tinst,m))
else None)

This comment has been minimized.

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan This is problably the most important fix. Previously { r with x = 1; y = 2 } was becoming { x=1; y=2; z= (COPY-OF-R).z; w = (COPY-OF-R).w } That is each old field induced a copy of r.

Now, we take the address of r once (making a silent copy in order to take the address if needed). Then the record expression becomes { x=1; y=2; z= (ADDRESS-OF-R)->z; w = (ADDRESS-OF-R)->w }

But more testing should be added around this, partly because the difference might be seen in quotations. Also a codegen EmittedIL test should probably be added for this case

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan This is problably the most important fix. Previously { r with x = 1; y = 2 } was becoming { x=1; y=2; z= (COPY-OF-R).z; w = (COPY-OF-R).w } That is each old field induced a copy of r.

Now, we take the address of r once (making a silent copy in order to take the address if needed). Then the record expression becomes { x=1; y=2; z= (ADDRESS-OF-R)->z; w = (ADDRESS-OF-R)->w }

But more testing should be added around this, partly because the difference might be seen in quotations. Also a codegen EmittedIL test should probably be added for this case

This comment has been minimized.

@TIHan

TIHan Jun 25, 2016

Contributor

This makes a lot of sense. I didn't think about this.

@TIHan

TIHan Jun 25, 2016

Contributor

This makes a lot of sense. I didn't think about this.

@@ -32,6 +32,10 @@ public class Lib2
public static FSharpRef<int> ri1 = new FSharpRef<int>(3);

This comment has been minimized.

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan You might want to add some struct-record testing to this fsfromfsviacs test. It both checks the access of F# code from C#, and checks the access of struct records across assembly boundaries. This is important because fields can be accessed directly within an assembly, but across assemblies they can't be and values must be accessed via helper methods instead.

@dsyme

dsyme Jun 17, 2016

Contributor

@TIHan You might want to add some struct-record testing to this fsfromfsviacs test. It both checks the access of F# code from C#, and checks the access of struct records across assembly boundaries. This is important because fields can be accessed directly within an assembly, but across assemblies they can't be and values must be accessed via helper methods instead.

This comment has been minimized.

@TIHan

TIHan Jun 25, 2016

Contributor

I agree. We should have tests checking the access.

@TIHan

TIHan Jun 25, 2016

Contributor

I agree. We should have tests checking the access.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 17, 2016

Contributor

@dotnet-bot test ci please

Contributor

dsyme commented Jun 17, 2016

@dotnet-bot test ci please

@dsyme dsyme closed this Jun 17, 2016

@dsyme dsyme reopened this Jun 17, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 17, 2016

Contributor

@dsyme same here. dotenet restore nreaks the build...

Contributor

forki commented Jun 17, 2016

@dsyme same here. dotenet restore nreaks the build...

@dsyme dsyme closed this Jun 20, 2016

@dsyme dsyme reopened this Jun 20, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 20, 2016

Contributor

Timeout on Appveyor - otherwise this is green

Contributor

dsyme commented Jun 20, 2016

Timeout on Appveyor - otherwise this is green

dsyme added some commits Jun 20, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 21, 2016

Contributor

This is ready for review and commit

Contributor

dsyme commented Jun 21, 2016

This is ready for review and commit

@KevinRansom KevinRansom merged commit a4faea5 into Microsoft:master Jun 25, 2016

4 checks passed

Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Jun 25, 2016

Contributor

Thanks for taking care of this.

Kevin

Contributor

KevinRansom commented Jun 25, 2016

Thanks for taking care of this.

Kevin

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Jun 25, 2016

Contributor

👍

Contributor

cartermp commented Jun 25, 2016

👍

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Jun 25, 2016

Contributor

This is all great. There is definitely a lot to learn from in this PR; which is awesome.

@dsyme thanks for the feedback. I need to look at what you mentioned closely.

Contributor

TIHan commented Jun 25, 2016

This is all great. There is definitely a lot to learn from in this PR; which is awesome.

@dsyme thanks for the feedback. I need to look at what you mentioned closely.

@jdh30

This comment has been minimized.

Show comment
Hide comment
@jdh30

jdh30 Jun 26, 2016

Contributor

This is great but... can we have struct unions that work for all (non-recursive) union types?

Contributor

jdh30 commented Jun 26, 2016

This is great but... can we have struct unions that work for all (non-recursive) union types?

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