Updating Number.Formatting to properly print -0 by tannergooding · Pull Request #19775 · dotnet/coreclr

tannergooding · 2018-08-30T22:02:27Z

This resolves https://github.com/dotnet/coreclr/issues/10290

tannergooding · 2018-08-30T22:03:41Z

FYI. @danmosemsft, @jkotas

This covers part of IEEE 754:2008 - 5.12.1: External character sequences representing zeros, infinities, and NaNs:

The conversions (described in 5.4.2) from supported formats to external character sequences and back that recover the original floating-point representation, shall recover zeros, infinities, and quiet NaNs, as well as non-zero finite numbers. In particular, signs of zeros and infinities are preserved

tannergooding · 2018-08-30T22:05:03Z

This fixes System.Double and System.Single. It also fixes System.Decimal, as that was the simpler change and I would believe is the desired behavior here as well.

This impacts 'R', as well as the other formats (such as 'G'). If we want to restrict this to just 'R', that will be a slightly more involved change.

tannergooding · 2018-08-30T22:16:16Z

Looks like linux-musl is using a C runtime without signbit support, will fix.

-- Looking at CoreFX there doesn't appear to be existing code coverage for "R" or some of the special-cases (tests are still running locally to confirm I didn't just miss them). I'll work on getting those cases covered.

tannergooding · 2018-08-31T18:49:07Z

src/pal/src/cruntime/math.cpp

+x  Double-precision floating-point value
+
+--*/
+int __cdecl PAL_signbit(double x)


Had to add PAL_signbit.... Clang complains about importing fp.h and using FPDOUBLE to get the sign-bit, because the header also declares a static function which is unused in grisu3.cpp

Rather than fix the header or compilation flags, I opted to just use signbit, since it is the standard library function to do this.

hmmm, looks like that won't work on all platforms, due to it being a #define sometimes (or I missed something)

You may need to add signbit to src/pal/src/include/pal/palinternal.h

Adding it to palinternal.h doesn't work in this case, because it is a macro in the math.h file.

I had to do what the other "macro" PAL redefinitions are doing (like isfinite and isnan), which is to define a new function (_signbit) which calls the macro, and then use the new function everywhere.
For the existing ones, the new function name was selected to match the MSVC function name (_finite, _isnan, etc). Since there is no _signbit function for MSVC (just signbit), I had to (for MSVC) also do #define signbit _signbit.

tannergooding · 2018-09-01T05:36:27Z

Will go through the various CoreFX failures and make sure they are all expected.
Will then prep the PR which fixes them to be "correct" for after this is merged and gets pumped over.

tannergooding · 2018-09-01T17:28:27Z

At least one of the failures is due to a bad change (specifically relating to integers). The break is in RoundNumber, which is used for rounding integers, decimals, and doubles/floats.

We are currently bypassing rounding (for decimal) in certain cases by passing an additional isDecimal parameter. There are a few ways I could fix this to keep the current behavior for integer and the new behavior for double/float. However, given that double/float also have special rounding rules (which RoundNumber doesn't currently handle), and that is may be good to generally track the original number "kind", I am working on a change that would allow NumberBuffer to carry around the "kind" (Unknown, Integer, Decimal, or Double) so that we can appropriately handle the various behaviors where necessary.

tannergooding · 2018-09-01T19:28:57Z

System.Memory.Tests failures are because the UTF8String formatter also needs to be updated.

tannergooding · 2018-09-04T21:27:22Z

Rebased to try and get jobs to run, most of them didn't kick off last time.

CoreFX side change is here: dotnet/corefx#32109

eerhardt · 2018-09-05T16:25:51Z

    internal unsafe ref struct NumberBuffer // needs to match layout of NUMBER in coreclr's src/classlibnative/bcltype/number.h

This comment seems out of date now. I don't see you updating the number.h file.

Refers to: src/System.Private.CoreLib/shared/System/Number.NumberBuffer.cs:16 in 5f66c17. [](commit_id = 5f66c17, deletion_comment = False)

eerhardt · 2018-09-05T16:40:23Z

src/System.Private.CoreLib/shared/System/Number.Formatting.cs

            char fmt = ParseFormatSpecifier(format, out int digits);
            int precision = FloatPrecision;
            NumberBuffer number = default;
+            number.kind = NumberBufferKind.Double;


Would it make more sense for NumberBufferKind.Double to be renamed to NumberBufferKind.FloatingPoint or similar? It feels odd to use Double when we are working with float/Single

Possibly, but I would like to do that as a follow-up PR.

Currently we use DoubleToNumber for both single and double.... We should probably either rename that as well, or add an explicit code-path for SingleToNumber

We are introducing the enum in this PR. Why not give it a good name from the start?

Because, until the the methods are split to handle Single explicitly, it is actually a NumberBuffer which contains a Double (Single is upcast in order to make the call) and so the name is more accurate as-is.

Ah, got it. That makes sense.

eerhardt

tannergooding · 2018-09-05T17:04:01Z

This comment seems out of date now. I don't see you updating the number.h file

Fixed, added comments clarifying the expected offset of each field as well.

tannergooding · 2018-09-06T04:26:32Z

Turns out the Windows x86 implementation was using a custom implementation written in assembly (that still used the x87 FPU stack).
I've removed that in the latest commit, in favor of the shared implementation under number.cpp (which was already used everywhere else).

tannergooding · 2018-09-06T16:28:52Z

@danmosemsft, @jkotas, @eerhardt.

I believe all feedback has been resolved, but the PR has changed a bit since some of your approvals. I'd appreciate it if you could give it another cursory glance and sign-off again if everything looks good.

danmoseley · 2018-09-06T17:45:22Z

Seems reasonable but this is not my area. It would be good for @jkotas to OK it.

jkotas · 2018-09-06T17:53:15Z

The invalidated CoreFX tests need to be disabled to make the CI green.

tannergooding · 2018-09-06T18:01:30Z

@jkotas, is there a file tracking those tests in CoreCLR, or do they need to be disabled in CoreFX directly?

jkotas · 2018-09-06T18:09:27Z

Add them to: https://github.com/dotnet/coreclr/blob/master/tests/CoreFX/CoreFX.issues.json

…in number.cpp

tannergooding · 2018-09-06T20:49:12Z

test Ubuntu x64 Checked jitstress1
test Ubuntu x64 Checked jitstress2

test Windows_NT x64 Checked jitstress1
test Windows_NT x64 Checked jitstress2

test Windows_NT x86 Checked jitstress1
test Windows_NT x86 Checked jitstress2

tannergooding · 2018-09-06T20:49:25Z

Fixed the name of the CoreFX tests in the json file.

tannergooding · 2018-09-06T22:50:15Z

@fiigii, the Avx2.Gather failures are unrelated. Could you take a look?

tannergooding · 2018-09-06T23:00:18Z

@jkotas, any other feedback?

If not, this should be good to merge, the only failures are in the JitStress1 and JitStress2 jobs and are unrelated to the PR.

fiigii · 2018-09-07T00:16:36Z

@tannergooding I will look into this failure tomorrow, thanks.

jkotas approved these changes Aug 30, 2018

View reviewed changes

tannergooding commented Aug 31, 2018

View reviewed changes

danmoseley approved these changes Sep 1, 2018

View reviewed changes

tannergooding mentioned this pull request Sep 4, 2018

Updating the UTF8Formatter to properly print -0 dotnet/corefx#32109

Closed

eerhardt reviewed Sep 5, 2018

View reviewed changes

eerhardt approved these changes Sep 5, 2018

View reviewed changes

tannergooding added 3 commits September 6, 2018 11:24

Updating Number.Formatting to properly print -0

409630e

Removing i386/fptext.asm in favor of using the shared implementation …

b7c3686

…in number.cpp

Disabling the failing CoreFX tests related to ToString for negative zero

d63afd8

tannergooding merged commit 98fa2d6 into dotnet:master Sep 7, 2018

fiigii mentioned this pull request Sep 7, 2018

Fix AVX2 Gather intrinsic failure with JITStress #19872

Merged

CarolEidt mentioned this pull request Sep 8, 2018

Improved fix for target register false dependency of lzcnt/tzcnt/popcnt #19811

Merged

This was referenced Sep 11, 2018

Adding a perf test for Double.ToString("R") and Single.ToString("R") #19887

Closed

Fix up the majority of IEEE compliance bugs for Double/Single ToString and Parse #19905

Closed

This was referenced Sep 23, 2018

Porting NumberToDouble to managed code. #20080

Merged

Updating NumberToStringFormat to not print the sign if there are no digits being returned #20109

Merged

ViktorHofer mentioned this pull request Sep 24, 2018

Revert "Add more VB operator tests" dotnet/corefx#32439

Merged

tannergooding mentioned this pull request Nov 9, 2018

Re-enabling some CoreFX parsing/formatting tests #20920

Closed

Conversation

tannergooding commented Aug 30, 2018

Uh oh!

tannergooding commented Aug 30, 2018

Uh oh!

tannergooding commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Sep 1, 2018

Uh oh!

tannergooding commented Sep 1, 2018

Uh oh!

tannergooding commented Sep 1, 2018

Uh oh!

tannergooding commented Sep 4, 2018

Uh oh!

eerhardt commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Sep 5, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

danmoseley commented Sep 6, 2018

Uh oh!

jkotas commented Sep 6, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

jkotas commented Sep 6, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

tannergooding commented Sep 6, 2018

Uh oh!

fiigii commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tannergooding commented Aug 30, 2018 •

edited

Loading

tannergooding Aug 31, 2018 •

edited

Loading