Merge fields of the same type on multi-case union structs #11718

kerams · 2021-06-23T21:43:09Z

A quick and (very) dirty proof of concept of implementing fsharp/fslang-suggestions#699. The thread contains many other potential improvements, but I'm solely concentrating on the original, approved suggestion here. The optimization kicks in when each case of a struct DU comprises one field of the same type.

Given

[<Struct>]
type X = 
    | A of a: string
    | B of b: string

Old emitted struct

using System;
using System.Collections;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Microsoft.FSharp.Core;

[Struct]
[DebuggerDisplay("{__DebugDisplay(),nq}")]
[CompilationMapping(SourceConstructFlags.SumType)]
[Serializable]
[StructLayout(0, CharSet = 4, Size = 1)]
public struct X : IEquatable<A.X>, IStructuralEquatable, IComparable<A.X>, IComparable, IStructuralComparable
{

	[CompilationMapping(SourceConstructFlags.UnionCase, 0)]
	public static A.X NewA(string _a)
	{
		return new A.X(_a, 0, false);
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	[DebuggerBrowsable(0)]
	public bool IsA
	{
		[CompilerGenerated]
		[DebuggerNonUserCode]
		get
		{
			return this.Tag == 0;
		}
	}

	[CompilationMapping(SourceConstructFlags.UnionCase, 1)]
	public static A.X NewB(string _b)
	{
		return new A.X(_b, 1, 0);
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	[DebuggerBrowsable(0)]
	public bool IsB
	{
		[CompilerGenerated]
		[DebuggerNonUserCode]
		get
		{
			return this.Tag == 1;
		}
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal X(string _a, int _tag, bool A_3)
	{
		this.a = _a;
		this.Tag = _tag;
	}

	[CompilationMapping(SourceConstructFlags.Field, 0, 0)]
	[CompilerGenerated]
	[DebuggerNonUserCode]
	public string a { [DebuggerNonUserCode] get; }

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal X(string _b, int _tag, byte A_3)
	{
		this.b = _b;
		this.Tag = _tag;
	}

	[CompilationMapping(SourceConstructFlags.Field, 1, 0)]
	[CompilerGenerated]
	[DebuggerNonUserCode]
	public string b { [DebuggerNonUserCode] get; }

	[CompilerGenerated]
	[DebuggerNonUserCode]
	[DebuggerBrowsable(0)]
	public int Tag { [DebuggerNonUserCode] get; }

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal object __DebugDisplay()
	{
		return ExtraTopLevelOperators.PrintFormatToString<FSharpFunc<A.X, string>>(new PrintfFormat<FSharpFunc<A.X, string>, Unit, string, string, string>("%+0.8A")).Invoke(this);
	}

	[CompilerGenerated]
	public override string ToString()
	{
		return ExtraTopLevelOperators.PrintFormatToString<FSharpFunc<A.X, string>>(new PrintfFormat<FSharpFunc<A.X, string>, Unit, string, string, A.X>("%+A")).Invoke(this);
	}

	[CompilerGenerated]
	public sealed override int CompareTo(A.X obj)
	{
		int tag = this._tag;
		int tag2 = obj._tag;
		if (tag != tag2)
		{
			return tag - tag2;
		}
		IComparer genericComparer;
		if (this.Tag == 0)
		{
			genericComparer = LanguagePrimitives.GenericComparer;
			return string.CompareOrdinal(this._a, obj._a);
		}
		genericComparer = LanguagePrimitives.GenericComparer;
		return string.CompareOrdinal(this._b, obj._b);
	}

	[CompilerGenerated]
	public sealed override int CompareTo(object obj)
	{
		return this.CompareTo((A.X)obj);
	}

	[CompilerGenerated]
	public sealed override int CompareTo(object obj, IComparer comp)
	{
		A.X x = (A.X)obj;
		int tag = this._tag;
		int tag2 = x._tag;
		if (tag != tag2)
		{
			return tag - tag2;
		}
		if (this.Tag == 0)
		{
			return string.CompareOrdinal(this._a, x._a);
		}
		return string.CompareOrdinal(this._b, x._b);
	}

	[CompilerGenerated]
	public sealed override int GetHashCode(IEqualityComparer comp)
	{
		int num;
		string text;
		if (this.Tag == 0)
		{
			num = 0;
			int num2 = -1640531527;
			text = this._a;
			return num2 + (((text == null) ? 0 : text.GetHashCode()) + ((num << 6) + (num >> 2)));
		}
		num = 1;
		int num3 = -1640531527;
		text = this._b;
		return num3 + (((text == null) ? 0 : text.GetHashCode()) + ((num << 6) + (num >> 2)));
	}

	[CompilerGenerated]
	public sealed override int GetHashCode()
	{
		return this.GetHashCode(LanguagePrimitives.GenericEqualityComparer);
	}

	[CompilerGenerated]
	public sealed override bool Equals(object obj, IEqualityComparer comp)
	{
		if (!LanguagePrimitives.IntrinsicFunctions.TypeTestGeneric<A.X>(obj))
		{
			return false;
		}
		A.X x = (A.X)obj;
		int tag = this._tag;
		int tag2 = x._tag;
		if (tag != tag2)
		{
			return false;
		}
		if (this.Tag == 0)
		{
			return string.Equals(this._a, x._a);
		}
		return string.Equals(this._b, x._b);
	}

	[CompilerGenerated]
	public sealed override bool Equals(A.X obj)
	{
		int tag = this._tag;
		int tag2 = obj._tag;
		if (tag != tag2)
		{
			return false;
		}
		if (this.Tag == 0)
		{
			return string.Equals(this._a, obj._a);
		}
		return string.Equals(this._b, obj._b);
	}

	[CompilerGenerated]
	public sealed override bool Equals(object obj)
	{
		return LanguagePrimitives.IntrinsicFunctions.TypeTestGeneric<A.X>(obj) && this.Equals((A.X)obj);
	}

	[DebuggerBrowsable(0)]
	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal int _tag;

	public static class Tags
	{

		public const int A = 0;

		public const int B = 1;
	}
}

New emitted struct

using System;
using System.Collections;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Microsoft.FSharp.Core;

[Struct]
[DebuggerDisplay("{__DebugDisplay(),nq}")]
[CompilationMapping(SourceConstructFlags.SumType)]
[Serializable]
[StructLayout(0, CharSet = 4, Size = 1)]
public struct X : IEquatable<A.X>, IStructuralEquatable, IComparable<A.X>, IComparable, IStructuralComparable
{

	[CompilationMapping(SourceConstructFlags.UnionCase, 0)]
	public static A.X NewA(string _a)
	{
		return new A.X(_a, 0, false);
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	[DebuggerBrowsable(0)]
	public bool IsA
	{
		[CompilerGenerated]
		[DebuggerNonUserCode]
		get
		{
			return this.Tag == 0;
		}
	}

	[CompilationMapping(SourceConstructFlags.UnionCase, 1)]
	public static A.X NewB(string _a)
	{
		return new A.X(_a, 1, 0);
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	[DebuggerBrowsable(0)]
	public bool IsB
	{
		[CompilerGenerated]
		[DebuggerNonUserCode]
		get
		{
			return this.Tag == 1;
		}
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal X(string _a, int _tag, bool A_3)
	{
		this.a = _a;
		this.Tag = _tag;
	}

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal X(string _a, int _tag, byte A_3)
	{
		this.a = _a;
		this.Tag = _tag;
	}

	[CompilationMapping(SourceConstructFlags.Field, 0, 0)]
	[CompilerGenerated]
	[DebuggerNonUserCode]
	public string a { [DebuggerNonUserCode] get; }

	[CompilerGenerated]
	[DebuggerNonUserCode]
	[DebuggerBrowsable(0)]
	public int Tag { [DebuggerNonUserCode] get; }

	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal object __DebugDisplay()
	{
		return ExtraTopLevelOperators.PrintFormatToString<FSharpFunc<A.X, string>>(new PrintfFormat<FSharpFunc<A.X, string>, Unit, string, string, string>("%+0.8A")).Invoke(this);
	}

	[CompilerGenerated]
	public override string ToString()
	{
		return ExtraTopLevelOperators.PrintFormatToString<FSharpFunc<A.X, string>>(new PrintfFormat<FSharpFunc<A.X, string>, Unit, string, string, A.X>("%+A")).Invoke(this);
	}

	[CompilerGenerated]
	public sealed override int CompareTo(A.X obj)
	{
		int tag = this._tag;
		int tag2 = obj._tag;
		if (tag != tag2)
		{
			return tag - tag2;
		}
		IComparer genericComparer;
		if (this.Tag == 0)
		{
			genericComparer = LanguagePrimitives.GenericComparer;
			return string.CompareOrdinal(this._a, obj._a);
		}
		genericComparer = LanguagePrimitives.GenericComparer;
		return string.CompareOrdinal(this._a, obj._a);
	}

	[CompilerGenerated]
	public sealed override int CompareTo(object obj)
	{
		return this.CompareTo((A.X)obj);
	}

	[CompilerGenerated]
	public sealed override int CompareTo(object obj, IComparer comp)
	{
		A.X x = (A.X)obj;
		int tag = this._tag;
		int tag2 = x._tag;
		if (tag != tag2)
		{
			return tag - tag2;
		}
		if (this.Tag == 0)
		{
			return string.CompareOrdinal(this._a, x._a);
		}
		return string.CompareOrdinal(this._a, x._a);
	}

	[CompilerGenerated]
	public sealed override int GetHashCode(IEqualityComparer comp)
	{
		int num;
		string a;
		if (this.Tag == 0)
		{
			num = 0;
			int num2 = -1640531527;
			a = this._a;
			return num2 + (((a == null) ? 0 : a.GetHashCode()) + ((num << 6) + (num >> 2)));
		}
		num = 1;
		int num3 = -1640531527;
		a = this._a;
		return num3 + (((a == null) ? 0 : a.GetHashCode()) + ((num << 6) + (num >> 2)));
	}

	[CompilerGenerated]
	public sealed override int GetHashCode()
	{
		return this.GetHashCode(LanguagePrimitives.GenericEqualityComparer);
	}

	[CompilerGenerated]
	public sealed override bool Equals(object obj, IEqualityComparer comp)
	{
		if (!LanguagePrimitives.IntrinsicFunctions.TypeTestGeneric<A.X>(obj))
		{
			return false;
		}
		A.X x = (A.X)obj;
		int tag = this._tag;
		int tag2 = x._tag;
		if (tag != tag2)
		{
			return false;
		}
		if (this.Tag == 0)
		{
			return string.Equals(this._a, x._a);
		}
		return string.Equals(this._a, x._a);
	}

	[CompilerGenerated]
	public sealed override bool Equals(A.X obj)
	{
		int tag = this._tag;
		int tag2 = obj._tag;
		if (tag != tag2)
		{
			return false;
		}
		if (this.Tag == 0)
		{
			return string.Equals(this._a, obj._a);
		}
		return string.Equals(this._a, obj._a);
	}

	[CompilerGenerated]
	public sealed override bool Equals(object obj)
	{
		return LanguagePrimitives.IntrinsicFunctions.TypeTestGeneric<A.X>(obj) && this.Equals((A.X)obj);
	}

	[DebuggerBrowsable(0)]
	[CompilerGenerated]
	[DebuggerNonUserCode]
	internal int _tag;

	public static class Tags
	{

		public const int A = 0;

		public const int B = 1;
	}
}

~~Extra ctors can be removed~~
~~Equals, CompareTo and GetHashCode can be simplified~~
~~Currently retains the field name specified in the first DU case, not sure if OK~~
~~Extend the optimization to work across multi-field cases by sharing the common fields but retaining the rest (Can this be done later, or is it better to do it now?)~~
~~Nullary cases should not prevent the optimization~~
~~Will old compilers be able to consume these structs and emit correct field getters without extra work? If not, what needs to be done?~~

~~So far it looks like it should be simple enough. Is there some glaring oversight on my part?~~

dsyme · 2021-06-24T12:02:42Z

This looks plausible.

Can't we emit a union struct (a struct with overlapping fields)? We ideally want to keep the name b

dsyme · 2021-06-24T12:06:38Z

The original surface area must be the same, so this property must appear:

	public string b { [DebuggerNonUserCode] get; }

even if its implementation goes to a shared field.

Also, we will need to review and add reflection tests for struct unions of this kind.

kerams · 2021-06-24T13:13:49Z

The original surface area must be the same, so this property must appear:

Got it.

Also, we will need to review and add reflection tests for struct unions of this kind.

I was surprised there were no IL tests for this that would've failed here.

Can't we emit a union struct (a struct with overlapping fields)?

Hm, yes, that would be ideal. However, I reckon messing around with explicit overlapping layout would be far trickier (fsharp/fslang-suggestions#699 (comment) and fsharp/fslang-suggestions#699 (comment)) than this seemingly straightforward approach. Unless you're opposed to it, I'd like to continue with what I'm doing. It could be a quick win and a (perhaps temporary) stepping stone towards a better solution.

dsyme · 2021-06-24T20:01:51Z

Hm, yes, that would be ideal. However, I reckon messing around with explicit overlapping layout would be far trickier than this seemingly straightforward approach. Unless you're opposed to it, I'd like to continue what I'm doing. It could be a quick win and a (perhaps temporary) stepping stone towards a better solution.

I'm not sure it would be that hard. The main thing is determining the exact restrictions on what works and what doesn't in CommonIL

dsyme · 2021-06-24T20:09:11Z

Unless you're opposed to it, I'd like to continue with what I'm doing. It could be a quick win and a (perhaps temporary) stepping stone towards a better solution.

Certainly it's ok to continue to get this green. Then I'd encourage you to try a spike to do the explicit fields.

kerams · 2021-06-26T13:53:10Z

Ok, I've decided to start over with explicit fields and in many ways it looks simpler than the original approach would be. I do have a couple of questions/problems though.

I need to tell apart value type and reference type fields because these can't overlap each other. I'm using tyconRef.IsStructOrEnumTycon, but it returns false for FSharp.Core.int fields (hence the failing test). What do I have to do here?
What's the debug proxy field? Does it come into play with struct unions? If so, I have to take it into account when calculating offsets.
How can I can I get the size in bytes of value types from TType or ILType instances?

kerams · 2021-06-26T14:07:59Z

Overlapping refence type fields of different types also lead to some strange behavior.

This snippet prints some undefined part of the process memory (or it throws System.AccessViolationException based on the value of Z). Not a problem for us because we're not accessing fields outside of the current union case context. On the other hand, the debugger/locals view craps out when expanding x, and this does affect us.

NinoFloris · 2021-06-26T14:22:36Z

@kerams overlapping ref types is unverifiable according to the ecma spec. You could maybe work around the debug issue with a debugtypeproxy but @dsyme I guess the question is if F# should produce unverifiable IL at all here ...

NinoFloris · 2021-06-26T18:37:03Z

Tangentially related issue and some background by @jkotas dotnet/roslyn#42617 (comment)

dsyme · 2021-06-29T11:30:41Z

Tangentially related issue and some background by @jkotas dotnet/roslyn#42617 (comment)

I see, I hadn't realised debugging would break. I suppose %A printing in FSI would also be exposed

One option would be to reduce the fields to object and emit type casts for the cases we don't know will succeed.

Another option would be to only share fields for identical types (typeEquiv)

I need to tell apart value type and reference type fields because these can't overlap each other. I'm using tyconRef.IsStructOrEnumTycon, but it returns false for FSharp.Core.int fields (hence the failing test). What do I have to do here?

isStructTy should do the trick

What's the debug proxy field? Does it come into play with struct unions? If so, I have to take it into account when calculating offsets.

Could you link to the source please? I'm not quite sure what you're referring to.

How can I can I get the size in bytes of value types from TType or ILType instances?

In the compiler you can't unfortunately, you'd need to write new logic that evaluates this in terms of the machine int size.

kerams · 2022-02-11T07:55:17Z

In the compiler you can't unfortunately, you'd need to write new logic that evaluates this in terms of the machine int size.

So I was thinkin sizeof could do the heavy lifting for us, but then I realized it would return sizes for the architecture the of the host the compiler is running on, but what we're after is the architecture we're compiling for. The PR seems to be too big of an undertaking if value type size computation needs to be implemented from scratch (and right now I don't even know where to start with that).

Merge fields of the same type on multi-case union structs

5132ffd

kerams force-pushed the struct branch from 564a90f to 5132ffd Compare June 26, 2021 11:32

Do not use explicit layout for single case struct unions

c7f6ea8

Correctly recognize value types

f050797

kerams closed this Apr 13, 2022

abelbraaksma mentioned this pull request Sep 17, 2022

Improve error message for Restriction to use named fields for a struct multi case DU #13900

Closed

edgarfgp mentioned this pull request Oct 5, 2022

Unclear error, FS3204: If a union type has more than one case and is a struct, then all fields within the union type must be given unique names #3648

Closed

kerams deleted the struct branch November 5, 2022 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge fields of the same type on multi-case union structs #11718

Merge fields of the same type on multi-case union structs #11718

kerams commented Jun 23, 2021 •

edited

Loading

dsyme commented Jun 24, 2021

dsyme commented Jun 24, 2021 •

edited

Loading

kerams commented Jun 24, 2021 •

edited

Loading

dsyme commented Jun 24, 2021

dsyme commented Jun 24, 2021

kerams commented Jun 26, 2021

kerams commented Jun 26, 2021 •

edited

Loading

NinoFloris commented Jun 26, 2021 •

edited

Loading

NinoFloris commented Jun 26, 2021

dsyme commented Jun 29, 2021

kerams commented Feb 11, 2022 •

edited

Loading

Merge fields of the same type on multi-case union structs #11718

Merge fields of the same type on multi-case union structs #11718

Conversation

kerams commented Jun 23, 2021 • edited Loading

dsyme commented Jun 24, 2021

dsyme commented Jun 24, 2021 • edited Loading

kerams commented Jun 24, 2021 • edited Loading

dsyme commented Jun 24, 2021

dsyme commented Jun 24, 2021

kerams commented Jun 26, 2021

kerams commented Jun 26, 2021 • edited Loading

NinoFloris commented Jun 26, 2021 • edited Loading

NinoFloris commented Jun 26, 2021

dsyme commented Jun 29, 2021

kerams commented Feb 11, 2022 • edited Loading

kerams commented Jun 23, 2021 •

edited

Loading

dsyme commented Jun 24, 2021 •

edited

Loading

kerams commented Jun 24, 2021 •

edited

Loading

kerams commented Jun 26, 2021 •

edited

Loading

NinoFloris commented Jun 26, 2021 •

edited

Loading

kerams commented Feb 11, 2022 •

edited

Loading