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

HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES # GetParam() = 536870912 (Subprocess aborted) #1549

Closed
malaterre opened this issue Jul 11, 2023 · 9 comments

Comments

@malaterre
Copy link
Contributor

[sorry for the flood of issues]

Thsi is to track test failure:

@malaterre
Copy link
Contributor Author

143: Test command: /home/malat/highway/bin/tests/math_test "--gtest_filter=HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES" "--gtest_also_run_disabled_tests"
143: Working Directory: /home/malat/highway/bin
143: Test timeout computed to be: 1500
143: Running main() from ./googletest/src/gtest_main.cc
143: Note: Google Test filter = HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES
143: [==========] Running 1 test from 1 test suite.
143: [----------] Global test environment set-up.
143: [----------] 1 test from HwyMathTestGroup/HwyMathTest
143: [ RUN      ] HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES
143: Mismatch for i=14 expected 2.356194 actual 3.141593
143: Abort at /home/malat/highway/hwy/contrib/math/math_test.cc:401: Assert 0
1/1 Test #143: HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES  # GetParam() = 536870912 ...Subprocess aborted***Exception:   0.01 sec

@malaterre
Copy link
Contributor Author

gcc-12:

73: Test command: /home/malat/highway/bin-gcc12/tests/math_test "--gtest_filter=HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES" "--gtest_also_run_disabled_tests"
73: Working Directory: /home/malat/highway/bin-gcc12
73: Test timeout computed to be: 1500
73: Running main() from ./googletest/src/gtest_main.cc
73: Note: Google Test filter = HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES
73: [==========] Running 1 test from 1 test suite.
73: [----------] Global test environment set-up.
73: [----------] 1 test from HwyMathTestGroup/HwyMathTest
73: [ RUN      ] HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES
73: [       OK ] HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES (0 ms)
73: [----------] 1 test from HwyMathTestGroup/HwyMathTest (0 ms total)
73:
73: [----------] Global test environment tear-down
73: [==========] 1 test from 1 test suite ran. (0 ms total)
73: [  PASSED  ] 1 test.
1/1 Test #73: HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES  # GetParam() = 536870912 ...   Passed    0.01 sec

The following tests passed:
        HwyMathTestGroup/HwyMathTest.TestAllAtan2/NEON_WITHOUT_AES  # GetParam() = 536870912

@malaterre
Copy link
Contributor Author

@jan-wassenberg could you remind me how I can refactor the math_test source code in this case.

I always find it confusing the lines:

#undef HWY_TARGET_INCLUDE
#define HWY_TARGET_INCLUDE "hwy/contrib/math/math_test.cc"
#include "hwy/foreach_target.h"  // IWYU pragma: keep

In my case I know I need to compile only against NEON_WITHOUT_AES . Thanks !

@jan-wassenberg
Copy link
Member

[sorry for the flood of issues]

I appreciate you taking the time to raise the issues!

So it seems gcc-13 only, gcc-12 is fine. I wasn't able to repro with gcc-12 either.

Here is the full version with dynamic dispatch: https://gcc.godbolt.org/z/bvMEhnxzK
You can delete those lines, here is a shorter example: https://gcc.godbolt.org/z/7xv64xoj6

Hope this helps :)

@malaterre
Copy link
Contributor Author

@malaterre
Copy link
Contributor Author

malaterre commented Jul 13, 2023

You can delete those lines, here is a shorter example: https://gcc.godbolt.org/z/7xv64xoj6

I did click on the link. I failed to understand how that could compile (automated default to EMU128 when not specified?)

GCC dev expect bug reporter to produce a reduce test case. Last time I manually extracted the math function into a smaller test case I could not reproduce the issue. I really need to use creduce for those. I need to manually remove the HWY_TARGET_INCLUDE trick to get creduce to work.

@jan-wassenberg
Copy link
Member

Ah, on aarch64 we always have NEON (without AES) so no flag required :)

In case the link is behaving strangely, here is the start of the file. Note that the math-inl header includes highway.h.

#include <hwy/contrib/math/math-inl.h>

namespace HWY_NAMESPACE {  // required: unique per target

// Can skip hn:: prefixes if already inside hwy::HWY_NAMESPACE.
namespace hn = hwy::HWY_NAMESPACE;

using T = float;

HWY_ATTR void MulAddLoop(const T* HWY_RESTRICT mul_array,
                const T* HWY_RESTRICT add_array,
                const size_t size, T* HWY_RESTRICT x_array) {

@pinskia
Copy link

pinskia commented Oct 4, 2023

IsInf depends on signed integer being wrapping incorrectly.
It would be better if it was written as:

template <typename T, size_t N, HWY_IF_FLOAT(T)>
HWY_API Mask128<T, N> IsInf(const Vec128<T, N> v) {
  const DFromV<decltype(v)> d;
  const RebindToUnsigned<decltype(d)> du;
  const VFromD<decltype(du)> vi = BitCast(du, v);
  // 'Shift left' to clear the sign bit, check for exponent=max and mantissa=0.
  return RebindMask(d, Eq(Add(vi, vi), Set(du, hwy::MaxExponentTimes2<T>())));
}

So not depending on signed integers here and use unsigned types which are defined to be wrapping.
Note other target codes has the same issue too.

@jan-wassenberg
Copy link
Member

Thank you @malaterre for sending the patch, and @pinskia for investigating and commenting here :)

I had thought that signed overflow/underflow was fine for vector (but not scalar) code, also because we do test with ubsan. Good to know that this isn't checking vector code.

After this patch lands, I will update the other IsInf as well, and look for MSB/Clear for similar occurrences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants