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

[SR-2868] Bug in significandWidth property of floating point types. #45462

Closed
hooman opened this issue Oct 5, 2016 · 9 comments
Closed

[SR-2868] Bug in significandWidth property of floating point types. #45462

hooman opened this issue Oct 5, 2016 · 9 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella

Comments

@hooman
Copy link
Contributor

hooman commented Oct 5, 2016

Previous ID SR-2868
Radar None
Original Reporter @hooman
Type Bug
Status Resolved
Resolution Done
Environment

Swift 3.0 release version. The current master branch shows that the bug is still there.

Additional Detail from JIRA
Votes 0
Component/s Standard Library
Labels Bug
Assignee @hooman
Priority Medium

md5: 3afb249b6c7734b3809a2e4ed7ad70e3

Issue Description:

UPDATE: I initially misunderstood the bug. I am revising the report to make it clear what is the issue and how to fix it.*

When significandBitPattern is zero, significandWidth should return zero, but it returns an unexpected value: -12 for Double and -9 for Float.

The reason for these specific numbers is that:

In the case of Double: countTrailingZeros returns 64 while significandBitCount is 52, hence the number -12 (= 52 - 64).

In the case of Float: countTrailingZeros returns 32 while significandBitCount is 23, hence the number -9 (= 23 - 32).

The proposed fix is guarding significandBitPattern != 0: (FloatingPointTypes.swift.gyb, lines 668 to 677)

   public var significandWidth: Int {
    guard significandBitPattern != 0 else { return 0 } // Added to fix SR-2868
    let trailingZeros = significandBitPattern.countTrailingZeros
    if isNormal {
      return ${Self}.significandBitCount - trailingZeros
    }
    if isSubnormal {
      return significandBitPattern.signBitIndex - trailingZeros
    }
    return -1
  }
  • I initially thought it was the result of the undefined behavior of Builtin.int_cttz_Int32/Builtin.int_cttz_Int64 when the input parameter is zero, but then realized the intrinsic is correctly called to create a defined behavior which is returning the bit width of the value.
@stephentyrone
Copy link
Member

`countTrailingZeros` should handle this.

@hooman
Copy link
Contributor Author

hooman commented Oct 20, 2016

I edited the bug report in response to the above comment, as changing `countTrailingZeros` by itself will not fix this particular bug. See the revised bug report. By the way, one alternative the my suggested fix for `countTrailingZeros` is to add a precondition that the value is not zero, instead of coming up with a definition for it as I did.

@stephentyrone
Copy link
Member

We want countTrailingZeros to return the type width for zero. This is the only sane definition (though it doesn't generalize to non fixed-width types). The LLVM intrinsic doesn't guarantee this for the historical reasons of the Intel BSF / BSR instructions, but the modern TZCNT and LZCNT instructions do the right thing, as do CTZ / CLZ on arm, so that's both the most sensible and the most performant choice.

@hooman
Copy link
Contributor Author

hooman commented Oct 20, 2016

So, should we file a bug against LLVM intrinsic? Because as I mentioned in the bug, the code that it generates, is not doing the right thing and returns garbage.

My suggested fix also does what you suggest (it returns the type width) but will be mush slower than if LLVM emitted the correct instruction and emulated it for architectures that don't support it directly.
Still, as I mentioned, fixing `countTrailingZeros` will not fix `significandWidth`.

@stephentyrone
Copy link
Member

No, the intrinsic is behaving as defined (this has been hashed out on the LLVM lists in the past). Your fix will not be slower than the intrinsic on platforms that support it directly, because the compiler knows how to eliminate the branch when targeting platforms where the raw instruction does the right thing.

@hooman
Copy link
Contributor Author

hooman commented Oct 22, 2016

OK, I just got a chance to refine my proposed fix for `countTrailingZeros` so that it actually compiles. I can assign the bug to myself and create a PR for my proposed fix, but I am new to stdlib development and don't know where to find documentation for `Builtin`s. So, I am not sure if I am doing it the correct way or not.

@hooman
Copy link
Contributor Author

hooman commented Nov 17, 2016

I Updated the bug report to correct my initial misunderstanding about the cause of the bug. Please re-read the bug description.

@hooman
Copy link
Contributor Author

hooman commented Nov 17, 2016

Submitted my proposed fix as pull request #5832 #5832

@stephentyrone
Copy link
Member

Merged fix as d7d05e1.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

2 participants