Repository navigation
GH-45083: [C++] Add HalfFloat kernels for is_nan, is_inf, is_finite, negate, negate_checked, sign - #46866
Conversation
github-actions
Bot
added
Component: C++
Component: Documentation
awaiting review
labels
fc77126 to
0a90874
Compare
|
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
|
I'll be out for a few days. Hopefully next week I can take a look. |
…nite, negate, negate_checked, sign Also improve the kernel generation infrastructure so as to make further half-float kernels easier to implement.
0a90874 to
17c0457
Compare
|
@github-actions crossbow submit -g cpp |
|
Rebased. |
|
Revision: 17c0457 Submitted crossbow builds: ursacomputing/crossbow @ actions-8bf71d8a31
|
|
|
||
| static bool CompareScalars(Scalar u, Scalar v) { | ||
| if (u.is_nan()) { | ||
| // XXX should we do a bit-precise comparison? |
There was a problem hiding this comment.
I think we should consider any two NaNs equivalent. Bit-precise comparison can be accomplished by viewing as uint16_t
There was a problem hiding this comment.
This is the same comment as for other FP types above. I think it should be a separate issue, because it's not obvious what the behaviour should really be.
There was a problem hiding this comment.
FWIW: I meant to say that I think the current behavior of considering all NaNs equivalent is correct and the comments should be removed.
There was a problem hiding this comment.
Ah, sorry for misunderstanding!
| using UnsignedIntegerTypes = | ||
| testing::Types<UInt8Type, UInt16Type, UInt32Type, UInt64Type>; | ||
|
|
||
| // TODO(kszucs): add half-float |
|
Thanks for the review @bkietz . I'll merge now, because changing the hashtable behavior of NaN values is more of an open question and should be a separate issue. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 2c51ceb. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
Half-float is currently supported by none of the arithmetic compute functions.
What changes are included in this PR?
Add half-float support for a small subset of arithmetic compute functions:
is_nan,is_inf,is_finite,negate,negate_checked,sign.Also improve the kernel generation infrastructure so as to make further half-float kernels easier to implement.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.
is_nanfor float16 #45083