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
Comments
`countTrailingZeros` should handle this. |
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. |
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. |
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. |
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. |
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. |
I Updated the bug report to correct my initial misunderstanding about the cause of the bug. Please re-read the bug description. |
Merged fix as d7d05e1. |
Environment
Swift 3.0 release version. The current master branch shows that the bug is still there.
Additional Detail from JIRA
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 forDouble
and -9 forFloat
.The reason for these specific numbers is that:
In the case of
Double
:countTrailingZeros
returns 64 whilesignificandBitCount
is 52, hence the number -12 (= 52 - 64).In the case of
Float
:countTrailingZeros
returns 32 whilesignificandBitCount
is 23, hence the number -9 (= 23 - 32).The proposed fix is guarding
significandBitPattern != 0
: (FloatingPointTypes.swift.gyb, lines 668 to 677)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.The text was updated successfully, but these errors were encountered: