Half: An IEEE 754 compliant float16 type #37630
Conversation
|
Tagging subscribers to this area: @tannergooding |
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
| // Well-defined and commonly used values | ||
| // | ||
|
|
||
| public static readonly Half Epsilon = new Half(EpsilonBits); // 5.9604645E-08 |
|
Closing this for a bit. I need to implement |
`IConvertible wasn't part of the reviewed/approved surface. It looks like all of the methods were removed from the proposal, but not the actual interface. Implementing |
Ah ok. I went through that exact same thought process, but assumed that |
|
Just noting, I believe you want to reopen the PR before pushing any new commits. I believe GitHub won't allow you to reopen the PR otherwise (although that may have changed). |
tannergooding
changed the title
| bool ret = false; | ||
| if (float.TryParse(s, style, provider, out float floatResult)) | ||
| { | ||
| result = (Half)floatResult; | ||
| ret = true; | ||
| } | ||
| else | ||
| { | ||
| result = default(Half); | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
| bool ret = false; | |
| if (float.TryParse(s, style, provider, out float floatResult)) | |
| { | |
| result = (Half)floatResult; | |
| ret = true; | |
| } | |
| else | |
| { | |
| result = default(Half); | |
| } | |
| return ret; | |
| bool ret = float.TryParse(s, style, provider, out float floatResult) | |
| result = ret ? (Half)floatResult : default(Half); | |
| return ret; |
? assume it makes the same code
There was a problem hiding this comment.
Rather than calling into float.TryParse we should add a Number.TryParseHalf method. The method would ultimately be the same as https://source.dot.net/#System.Private.CoreLib/Number.Parsing.cs,1792 but it would return Half.PositiveInfinity, Half.NegativeInfinity, Half.NaN, and use a new HalfNumberBufferLength constant (this should be 21 if I did my math correctly).
There would then also be a new NumberToHalf method which likewise mirrors https://source.dot.net/#System.Private.CoreLib/Number.Parsing.cs,2002, but passes in FloatingPointInfo.Half. The rest of the logic should already be correct and should not require any touchups.
Similar should be true for the formatting code paths.
| /// </summary> | ||
| [Serializable] | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| public readonly struct Half : IComparable, IFormattable, IComparable<Half>, IEquatable<Half>, ISpanFormattable |
There was a problem hiding this comment.
@TamarChristinaArm, I was llooking at the ARM calling convention for half-precision floating point and it looks like its treated as a ushort rather than as a floating-point type for the purposes of argument passing/returning, is that correct?
A Half-precision Floating Point Type is returned in the least significant 16 bits of r0.
If the argument is a Half-precision Floating Point Type its size is set to 4 bytes as if it had been copied to the least significant bits of a 32-bit register and the remaining bits filled with unspecified values.
There also seems to be a section on variants of the calling convention which details:
A half precision floating point type is passed as if it were loaded from its memory format into the least significant 16 bits of a single precision register.
There was a problem hiding this comment.
its treated as a ushort rather than as a floating-point type for the purposes of argument passing/returning, is that correct?
@tannergooding passing it in GPR is only for the base variant, i.e. soft or softfp float ABI where you don't have hardware support for floats. When you have hardware support and use the hard float ABI it uses the VFP calling convention which passes it in the s register as the second part you quoted.
I'm guessing coreclr only supports hard? in which case the VFP one is what you want :).
There was a problem hiding this comment.
@CarolEidt, @sandreenko, @jkotas
Given the recent changes around retyping, what would the recommended way be to support this in the JIT/VM?
We could simply block this from interop and AOT scenarios for .NET 5 and address it in .NET 6...
But, if the changes aren't terribly difficult, it would be nice to also ensure it works in those scenarios.
As per the above, it looks like no changes are required for x86, x64, or System V as they have defined no ABI semantics for a half-precision floating-point type so any changes would only be required for ARM32/ARM64 where the ABI specifies it should be passed in the lowest 16-bits of a floating-point register.
There was a problem hiding this comment.
@tannergooding we need to know the type when we call and when we return, and without retyping all logic is located in Compiler::getReturnTypeForStruct, so necessary changes would be localized in the JIT inside this function and functions that are called by it.
I think it could be done similar to #37499 by adding a new type into VM and saying that we are returning it as a float it JIT, lower will add bit casts as necessary.
Do you expect methods that are using private readonly ushort m_value; directly, like:
public static bool IsPositiveInfinity(Half value)
{
return value.m_value == PositiveInfinityBits;
}
would get value as float, but access its field as a short that would cause a memory spill?
There was a problem hiding this comment.
Thanks for the explanation!
would get value as float, but access its field as a short that would cause a memory spill?
We have similar logic for System.Single and Siystem.Double: https://source.dot.net/#System.Private.CoreLib/Single.cs,79
Prior to .NET 5 it did cause a memory spill but I believe it now just emits a movd to directly transition between the floating-point and integer registers.
#11413 tracks the more general issue there and is something we could workaround for the type on ARM32/ARM64 as well.
I don't believe it will be a problem for x86/x64 as none of the architectures we support define an ABI for Half and so passing it around as an integer is as expected.
There was a problem hiding this comment.
So ... I could probably look this up, but I'm guessing you already know, so I'll ask here: Is Half considered a floating point type for the purposes of HFA recognition? If so, I think there might be a bit more work in the JIT to handle non-singletons correctly.
There was a problem hiding this comment.
The short answer is yes. The actual handling of ARM32 vs ARM64 appears to differ slightly and the docs on ARM64 appear to be quite a bit clearer.
The ARM64 specs are apparently on GitHub now 😄: https://github.com/ARM-software/abi-aa/releases
See also: https://developer.arm.com/architectures/system-architectures/software-standards/abi
| /// </summary> | ||
| [Serializable] | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| public readonly struct Half : IComparable, IFormattable, IComparable<Half>, IEquatable<Half>, ISpanFormattable |
There was a problem hiding this comment.
I also checked the System V ABI and x86/x64 Windows ABIs and did not see any notes on special handling required for a half or fp16 type.
src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs
Outdated
Show resolved
Hide resolved
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| internal static unsafe short HalfToInt16Bits(Half value) | ||
| { | ||
| return *((short*)&value); |
There was a problem hiding this comment.
Just wanted to call out it looks like the JIT does the right thing here: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYGuDFA5gMDABLZJAMxoBvGgwdNdsbABMIXSQE89AC2gmAPrAvBjqNAC+mrS63LjYljBMpAxCDHbUjk5+AQwAsuQAFOZWDL4AlOn2WY7EAOxlLMGh4ZmOUW0O1Q46OVAmeaTFFpZllRk1tQ0AVIWFuP790+UAZBUanQwdWd3ZC7l52sOlFVWbWfUMAKpc8YksAIK4ADwllmh9GAB8hbCj67sOhEgA===
src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs
Outdated
Show resolved
Hide resolved
|
LGTM now. Just a few nits and one fix left. |
|
A few of the test "expected" strings need to be updated to account for
|
|
@jkotas I think I need signoff from you too. Or GH won't let me merge |
|
Nice work @pgovind ..glad to get this into 5.0 |
Fixes dotnet#936. A lot of this code is a port of what have in [corefxlab](https://github.com/dotnet/corefxlab/tree/master/src/System.Numerics.Experimental).
Fixes #936. A lot of this code is a port of what have in [corefxlab](https://github.com/dotnet/corefxlab/tree/master/src/System.Numerics.Experimental).
| public static System.Half Epsilon { get { throw null; } } | ||
| public static System.Half MaxValue { get { throw null; } } | ||
| public static System.Half MinValue { get { throw null; } } | ||
| public static System.Half NaN { get { throw null; } } | ||
| public static System.Half NegativeInfinity { get { throw null; } } | ||
| public static System.Half PositiveInfinity { get { throw null; } } |
There was a problem hiding this comment.
Shouldn't these values be constants?
There was a problem hiding this comment.
They can't be actual constants either because constant can only be a limited set of types in IL and in C#.
ghost
locked as resolved and limited conversation to collaborators
Fixes #936.
A lot of this code is a port of what have in corefxlab.