[Bug go/63269] New: libgo/math test failures in TestLog2

[Bug go/63269] New: libgo/math test failures in TestLog2

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

            Bug ID: 63269
           Summary: libgo/math test failures in TestLog2
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: go
          Assignee: ian at airs dot com
          Reporter: vogt at linux dot vnet.ibm.com
                CC: cmang at google dot com

There are several flaws in the math library test code.  On s390x, the test
TestLog2 fails for three reasons:

1) This part of the test is too strict for i == 0:

for i := -1074; i <= 1023; i++ { 
  f := Ldexp(1, i) 
  l := Log2(f) 
  if l != float64(i) { 
    t.Errorf("Log2(2**%d) = %g, want %d", i, l, i) 
  } 
} 

It should really use the veryclose function() as there is no guarantee that the
result of this calculation from log10.go is exact:

  return Log(frac)*(1/Ln2) + float64(exp)

With frac == 0.5 and exp == 1, the result may be just close to 0, not exactly
zero:

* If the compiler generated two instructions, one to mulitply and one to add,
the result is (with a small error delta):

  Product: Log(0.5)*(1/Ln2) = -1 + delta -> rounded to -1
  Sum: -1 + 1 = 0 -> rounded to 0
  Result: 0

* If the compiler generates a single multiply and add instruction, only the
final result is rouced, i.e.

  Product: Log(0.5)*(1/Ln2) = -1 + delta (not rounded)
  Sum: -1 + delta + 1 = delta -> rounded to delta'
  Result: delta' != 0

So, the fix is to use the "veryclose" error tolerance.

2) There's a bug in the tolerance() function:

        if a != 0 { 
                e = e * a 
                if e < 0 { 
                        e = -e 
                } 
        } 

This snippet uses the defective value a but should use the expected value b
instead:

        if b != 0 { 
                e = e * b 
                if e < 0 { 
                        e = -e 
                } 
        } 

Otherwise, bad things happen for an expected value 0 when the defective value
is very close, i.e. they never match.

3) The alike() function does allow an error tolerance.  This kicks in on s390x
in the case that the expected value is 0 and the defective value is not
identical.

        switch { 
        case IsNaN(a) && IsNaN(b): 
                return true 
        case a == b: 
                return Signbit(a) == Signbit(b) 
        } 


A possible fix would be to detect this one situation and return true, and rely
on more specific tests to chack that the error is small ebough.

        switch { 
        case IsNaN(a) && IsNaN(b): 
                return true 
        case a == 0 && !IsNaN(b) && !IsInf(b, 0): 
                // allow deviations when the expected value is zero
                return true 
        case a == b: 
                return Signbit(a) == Signbit(b) 
        }

[Bug go/63269] libgo/math test failures in TestLog2

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

--- Comment #1 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Created attachment 33493
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33493&action=edit
Proposed fixes

[Bug go/63269] libgo/math test failures in TestLog2

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

--- Comment #2 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Created attachment 33506
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33506&action=edit
updated patch

Managed to get a stray character into the test between testing and committing
it.  Fixed.

[Bug go/63269] libgo/math test failures in TestLog2

[ian at airs dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

Ian Lance Taylor <ian at airs dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2014-11-05
     Ever confirmed|0                           |1

--- Comment #3 from Ian Lance Taylor <ian at airs dot com> ---
First, let me say that this code is in the Go master library and must be fixed
there.  It might be more effective to discuss it on the Go issue tracker at
http://golang.org/issue.

I don't agree with your argument for item 1.  You say that the wrong result
happens when using a fused multiply-add instruction.  The math library is
compiled with -ffp-contract=off (see MATH_FLAG in configure.ac and
Makefile.am), so the compiler should not be generating a fused multiply-add
instruction.

I'm not entirely persuaded by your argument for item 2.  Zero is a special
value.  When we expect a zero, we should get a zero, not something close to
zero.  I don't think this change is correct in general.  It may be correct for
some specific cases, but then we need to investigate those.

Item 3 is the same sort of thing: when we expect zero, we should, in general,
get exactly zero.

[Bug go/63269] libgo/math test failures in TestLog2

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

--- Comment #4 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
regarding 2)

> I'm not entirely persuaded by your argument for item 2. ...

Hm, good that you doubted it, because the actual mistake is somehwere else: 
The unpatched code has

  if l != float64(i)

but if you want to use a tolerance here this must become

  if !veryclose(float64(i), l) {

With the argument reversed.  This could/should be cleaned up by renaming the
arguments of the tolerance() function, e.g. a -> expected, b -> result, e ->
maxerr.

> Zero is a special
> value.  When we expect a zero, we should get a zero, not something close to
> zero.  I don't think this change is correct in general.  It may be correct for
> some specific cases, but then we need to investigate those.

Actually, this has nothing to do with 0 being special here, abut with scaling
of the allowed error: Multiplying it by 0 yields zero error tolerance, so the
tolerance() function does not do that.

--> This chunk is not necessary, but a (separate) cleanup patch might help to
avoid future confusion.

[Bug go/63269] libgo/math test failures in TestLog2

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

--- Comment #5 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
regarding 1)

My earlier explanation of the problem was wrong.  Multiply and add is not
generated; it probably only was in the artificial test case that I made and
certainly did not compile with -ffp-contract=off.

In this calculation in log2(),

  Log(frac)*(1/Ln2) + float64(exp)

Gcc does constant folding for (1/Ln2) and generates a multiply instruction and
then adds the second term.  Same result if you write "*Log2E" instead of
"*(1/Ln2)").  But with

  Log(frac)/Ln2 + float64(exp)

it generates a divide instruction.  The multiplication and the division yield
results that differ in the least significant bit, and I don't see how this
could be prevented in general; it's just an artifact of the floating point
format.  I've verified that the constants Ln2, 1/Ln2 and Log2E are bit correct.

The "easy" way to fix this is increasing the allowed tolerance as my patch does
(note that the arguments of the veryclose() call need to be swapped, see
previous comment).

The "right" way to fix this is to calculate platform specific ULPs for all the
algorithms from the math library and use these.  That's what glibc does.

[Bug go/63269] libgo/math test failures in TestLog2

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63269

--- Comment #6 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
(In reply to Ian Lance Taylor from comment #3)
> First, let me say that this code is in the Go master library and must be
> fixed there.  It might be more effective to discuss it on the Go issue
> tracker at http://golang.org/issue.

--> https://code.google.com/p/go/issues/detail?id=9066