cyb70289 commented on issue #12681: URL: https://github.com/apache/arrow/issues/12681#issuecomment-2490255120
Looks to me it's a compiler bug. The point is from below code line in [aggregate_test.cc](https://github.com/apache/arrow/blob/apache-arrow-16.1.0/cpp/src/arrow/compute/kernels/aggregate_test.cc#L4020-L4021). The same equation is used at [quantile kernel](https://github.com/apache/arrow/blob/apache-arrow-16.1.0/cpp/src/arrow/compute/kernels/aggregate_quantile.cc#L241) side. It calculates `fraction * input[lower_index + 1] + (1 - fraction) * input[lower_index]` Floating point addition is not commutative (i.e., a+b != b+a), it's important we do not swap the two addends. But from below disassembled code, if GLIBCXX_ASSERTIONS is enabled, the generated code swaps the two addends. **with `-Wp,-D_GLIBCXX_ASSERTIONS`:** **wrong**: calculates `(1 - fraction) * input[lower_index] + input[lower_index+1] * fraction` ``` 47c3d4: 1e6e1000 fmov d0, #1.000000000000000000e+00 47c3d8: 1e7f3800 fsub d0, d0, d31 // d0 = 1 - fraction 47c3dc: fc606abe ldr d30, [x21, x0] // d30 = input[lower_index+1] 47c3e0: 5e61dbde scvtf d30, d30 47c3e4: 1e7f0bdf fmul d31, d30, d31 // d31 = d30 * fraction = input[lower_index+1] * fraction 47c3e8: eb01033f cmp x25, x1 47c3ec: 54fffba9 b.ls 47c360 47c3f0: 8b0002a1 add x1, x21, x0 47c3f4: aa1403e0 mov x0, x20 47c3f8: fc5f803e ldur d30, [x1, #-8] // d30 = input[lower_index] 47c3fc: 5e61dbde scvtf d30, d30 47c400: 1f5e7c00 fmadd d0, d0, d30, d31 // d0 = d0 * d30 + d31 = (1 - fraction) * input[lower_index] + input[lower_index+1] * fraction ``` **without `-Wp,-D_GLIBCXX_ASSERTIONS`:** **correct**: calculates `fraction * input[lower_index+1] + input[lower_index] * (1 - fraction)` ``` 47b6a4: fc616a7f ldr d31, [x19, x1] // d31 = input[lower_index] 47b6a8: 1e6e101d fmov d29, #1.000000000000000000e+00 47b6ac: fd40041e ldr d30, [x0, #8] // d30 = input[lower_index+1] 47b6b0: 1e603bbd fsub d29, d29, d0 // d29 = 1 - fraction 47b6b4: 5e61dbff scvtf d31, d31 47b6b8: aa1603e0 mov x0, x22 47b6bc: 5e61dbde scvtf d30, d30 47b6c0: 1e7d0bff fmul d31, d31, d29 // d31 = input[lower_index] * (1 - fraction) 47b6c4: 1f5e7c00 fmadd d0, d0, d30, d31 // d0 = d0 * d30 + d31 = fraction * input[lower_index+1] + input[lower_index] * (1 - fraction) ``` I tried deliberately swap the two addends at the [quantile kernel](https://github.com/apache/arrow/blob/apache-arrow-16.1.0/cpp/src/arrow/compute/kernels/aggregate_quantile.cc#L241) side, the test passed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
