Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cast[uint](NaN) differs at CT vs RT; `-NaN and NaN have same bit representation #499

Open
timotheecour opened this issue Dec 29, 2020 · 21 comments · Fixed by nim-lang/Nim#16628

Comments

@timotheecour
Copy link
Owner

timotheecour commented Dec 29, 2020

Example

when true:
  import math
  proc main =
    let a1 = NaN
    let a2 = -NaN
    doAssert isNaN(a1)
    doAssert isNaN(a2)
    echo cast[uint](a1)
    echo cast[uint](a2)
  static: main()
  main()

Current Output

CT:
9221120237041090559
18444492273895866367
RT:
9221120237041090560
9221120237041090560

Expected Output

  • CT should match RT
  • for cast[uint](a1) vs cast[uint](a2) being the same, this seems odd; IMO we should just flip the sign bit regardless of NaN vs not NaN; we can also see what other langs do

Additional Information

devel 1.5.1
6d442a4

@ringabout
Copy link
Collaborator

I will check it.

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 6, 2021

my gut feeling is that the problem is the magic proc - is wrong for NaN

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

see the generated C codes:

	NF a1;
	NF a2;
	a1 = NAN;
	a2 = NAN;

And JS codes

    var x1_369098755 = NaN;
    var x2_369098756 = NaN;
    var a1_369098757 = x1_369098755;
    var a2_369098758 = x2_369098756;

C backend generates NAN for NAN or -NAN, but VM doesn't.

@ringabout
Copy link
Collaborator

my gut feeling is that the problem is the magic proc - is wrong for NaN

I think so.

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

Unfortunately:

import std/math


proc main =
  let x1 = NaN
  let x2 = -x1
  let a1 = float64(x1)
  let a2 = float64(x2)

  doAssert isNaN(a1)
  doAssert isNaN(a2)
  echo cast[uint](a1)
  echo cast[uint](a2)
static: main()
main()

VM:
9221120237041090560
18444492273895866368
Backend:
9221120237041090560
18444492273895866368

So it may be called a bug.

@timotheecour
Copy link
Owner Author

why close? the point of this issue is to track this bug; NaN should behave in CT like in RT, there are many numbers that are isNaN and each have their own (different) cast[uint], etc

@ringabout ringabout reopened this Jan 6, 2021
@ringabout
Copy link
Collaborator

Sorry, I click the wrong button.

@ringabout
Copy link
Collaborator

The suspicious parts:

  of nkFloatLit, nkFloat64Lit:
    result = rope(n.floatVal.toStrMaxPrecision)
  of nkFloat32Lit:
    result = rope(n.floatVal.toStrMaxPrecision("f"))

@ringabout ringabout reopened this Jan 6, 2021
@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

So only float literals are handled in C/JS backend:

proc toStrMaxPrecision*(f: BiggestFloat, literalPostfix = ""): string =
  case classify(f)
  of fcNan:
    result = "NAN"
  of fcNegZero:
    result = "-0.0" & literalPostfix
  of fcZero:
    result = "0.0" & literalPostfix
  of fcInf:
    result = "INF"
  of fcNegInf:
    result = "-INF"
  else:
    result = newString(81)
    let n = c_snprintf(result.cstring, result.len.uint, "%#.16e%s", f, literalPostfix.cstring)
    setLen(result, n)

@ringabout
Copy link
Collaborator

What do you think of NaN literals and NaN exprs? Should them differ or not? Should NaN differ from -NaN?

let x = NaN
let y = -x

vs

let x = NaN
let y = -NaN

@timotheecour
Copy link
Owner Author

timotheecour commented Jan 6, 2021

let b = -NaN should give same result as:

let a = NaN
let b = -a

anything else makes no sense.

proc identity[T](a: T): T =
  result = a
proc main()=
  let a = NaN
  let b = -a
  echo cast[uint](a) # ok
  echo cast[uint](b) # ok
  let c1 = -NaN
  echo cast[uint](c1) # bug
  let c = -(NaN)
  echo cast[uint](c) # bug
  let d = -identity(NaN)
  echo cast[uint](d) # ok
main()

9221120237041090560
18444492273895866368
9221120237041090560
9221120237041090560
18444492273895866368

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

signbit should be able to solve this problem, though I'm not sure about JS backend.

@ringabout
Copy link
Collaborator

After a fix:

import std/math


proc main =
  let x1 = NaN
  let x2 = -NaN
  let x3 = -x1

  doAssert isNaN(x1)
  doAssert isNaN(x2)
  doAssert isNaN(x3)
  echo cast[uint](x1)
  echo cast[uint](x2)
  echo cast[uint](x3)

static: main()
main()
9221120237041090559
18444492273895866367
18444492273895866367

9221120237041090560
18444492273895866368
18444492273895866368

@timotheecour
Copy link
Owner Author

perfect! for js backend, copySign should work no?

@timotheecour
Copy link
Owner Author

note: good that it gives same results as python3 (and probably C, haven't checked but there's no reason it shouldn't work since ctypes is based on C):

from math import nan

def fun(x):
  print()
  print(x)

  import ctypes
  u64 = ctypes.c_uint64.from_buffer(ctypes.c_double(x))
  print(u64)
  print(bin(u64.value))

fun(1.0)
fun(0.0)
fun(-0.0)
fun(nan)
fun(-nan)
1.0
c_ulong(4607182418800017408)
0b11111111110000000000000000000000000000000000000000000000000000

0.0
c_ulong(0)
0b0

-0.0
c_ulong(9223372036854775808)
0b1000000000000000000000000000000000000000000000000000000000000000

nan
c_ulong(9221120237041090560)
0b111111111111000000000000000000000000000000000000000000000000000

nan
c_ulong(18444492273895866368)
0b1111111111111000000000000000000000000000000000000000000000000000

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

copysign doesn't handle nan in JS backend and it hasn't worked now.

  template impl() =
    if y > 0.0 or (y == 0.0 and 1.0 / y > 0.0):
      result = abs(x)
    elif y <= 0.0:
      result = -abs(x)
    else: # must be NaN
      result = abs(x)

signbit does work in JS backend though.

@timotheecour
Copy link
Owner Author

copysign doesn't handle nan in JS backend and it hasn't worked now.

=> made it work here: nim-lang#16609

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

yeah, I mean JS backend also works(testing using signbit)

@ringabout
Copy link
Collaborator

ringabout commented Jan 6, 2021

copysign doesn't handle nan in JS backend and it hasn't worked now.

=> made it work here: nim-lang#16609

this PR should defer until MY signbit PR is merged, then copySign could reuse parts of signbits.

@timotheecour
Copy link
Owner Author

i agree, I've already LGTM'd nim-lang#16592, waiting for 2nd LGTM so we can merge it

@timotheecour
Copy link
Owner Author

@xflywind looks like the problem is still there even after nim-lang#16628 is merged ... (PR is good but looks like we need something additional)

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

Successfully merging a pull request may close this issue.

2 participants