tests: Avoid UB with abs of minimum integral value

[hahnjo]

The most negative number doesn't have an absolute value that can
be represented in the same type.

[mattkretz]

But it's not UB, which is why this corner case should still be tested.

short a = -32768;
short b = std::abs(a);

means static_cast<short>(std::abs(static_cast<int>(a))) and the conversion from int to short is "the result is the unique value of the destination type that is congruent to the source integer modulo 2N, where N is the width of the destination type". The width N is 16 for short. Thus the int 32768 (0x8000) becomes -32768. This is mad but not UB. Why Clang 11 produces a 32767 for the reference value is a mystery — but might be a bug either in Clang or the test code. What's important is that Vc produces the correct result and thus this specific test failure shouldn't block a release.

[hahnjo]

C++11, C++14, and C++17 seem to disagree, the relevant passage reads:

If the destination type is signed, the value is unchanged if it can be represented in the destination type (and bit-field width); otherwise, the value is implementation-defined.

So strictly speaking, my classification as UB is wrong but a test relying on implementation-defined behavior should still be fixed. If you agree that this should be fixed, I'll change the commit message.

[hahnjo]

For the record, the change that removed the above text is cplusplus/draft@3b7ee00. P1236R1 is marked as done in Clang 9, so I still don't understand where 32767 comes from...

[hahnjo]

Ah, I briefly forgot one important point: While the test failure is about short, the int code could definitely trigger UB in the call to abs:

Trying to take the absolute value of the most negative integer is not defined.

[mattkretz]

Right. After we made signed integers be two's complement in C++20 there's no need for implementation-defined behavior here anymore. But implementation-defined means the compiler must document the behavior and stick to it. For x86 that behavior is whatever the hardware does for all the compilers I checked. Yes, for int I agree it shouldn't test abs(INT_MIN).

[mattkretz]

To motivate testing & supporting this corner case: if you port scalar code over to Vc you want as few silent behavior changes as possible. Even if the use of abs(SHORT_MIN) is likely a bug, I'd rather have it behave as much as scalar short would behave.

[hahnjo]

I understand that, but please note that there's no guarantee that SHORT_MIN is actually tested: the vector comes from Random().

[mattkretz]

yes, I've seen that and it might be a good idea to test SHORT_MIN explicitly even

[hahnjo]

I think what's really required here is to understand where the value 32767 comes from. I've tried to reproduce this locally with a custom build of Clang 11, both with libstdc++ and libc++, but never got a failing test.

[mattkretz]

https://godbolt.org/z/hGsaY1 It may be due to an old libstdc++ that clang 11 is using on this machine

[hahnjo]

https://godbolt.org/z/hGsaY1

That's with GCC 6.3.0? I don't get this with Clang 11.

It may be due to an old libstdc++ that clang 11 is using on this machine

AFAICT Clang expands the call to abs with optimizations enabled: https://github.com/llvm/llvm-project/blob/release/11.x/clang/lib/CodeGen/CGBuiltin.cpp#L1969

[mattkretz]

found it. Clang 11 autovectorizes the scalar abs, which produces the reference vector. It uses the following sequence:

vpmovsxwd ymm1,xmm0                                                                                                                                                                                                                                                                               
vextracti128 xmm2,ymm1,0x1                                                                                                                                                                                                                                                                        
vcvtdq2pd ymm2,xmm2                                                                                                                                                                                                                                                                               
vcvtdq2pd ymm1,xmm1                                                                                                                                                                                                                                                                               
vmovupd ymm3,YMMWORD PTR [rsp+0x80]                                                                                                                                                                                                                                                               
vandpd ymm1,ymm1,ymm3                                                                                                                                                                                                                                                                             
vandpd ymm2,ymm2,ymm3                                                                                                                                                                                                                                                                             
vcvttpd2dq xmm2,ymm2                                                                                                                                                                                                                                                                              
vcvttpd2dq xmm1,ymm1                                                                                                                                                                                                                                                                              
vpackssdw xmm1,xmm1,xmm2

So it converts to two AVX double vectors, evaluates abs, converts back to int and then via saturating packing to short. Seems like the abs(int) overload is not visible in the test code?

[mattkretz]

Adding static_assert(std::is_same<decltype(std::abs(int())), int>::value, ""); to the test doesn't make it ill-formed. I have no idea why Clang vectorizes to this sh*tty code. A simple CE testcase produces vpabsw, as expected.

[hahnjo]

Hm, I don't see this locally either. I'm looking at Tests::testAbs_<Vc_1::Vector<short, Vc_1::VectorAbi::Sse> >::run() which a) uses the expected xmm registers for SSE and b) has the vpabsw instruction.

[mattkretz]

old libstdc++ is the problem

[mattkretz]

I'll merge your fix and will add:

    using T = typename V::value_type;                                                               
    if (std::is_same<T, short>::value)                                                              
    {                                                                                               
        const V a = std::numeric_limits<T>::lowest();                                               
        COMPARE(abs(a), a);                                                                         
    }

[hahnjo]

old libstdc++ is the problem

In fact I think they provided a templated version of std::abs: gcc-mirror/gcc@37b204d#diff-0067db1a1416cdc2ed5ba8d69cfd097c6a30a86aa35c3b6ec61c8e9ccff81453L99-L105 I remembered something like this, but couldn't find it in the documentation and didn't follow this...

[mattkretz]

My addition leads to failure on old libstdc++ when a Vc::Vector with scalar ABI tag is tested. Because its abs function uses the incorrect std::abs. I guess I'll add a & V::size() > 1 to the condition.

[hahnjo]

My addition leads to failure on old libstdc++ when a Vc::Vector with scalar ABI tag is tested. Because its abs function uses the incorrect std::abs. I guess I'll add a & V::size() > 1 to the condition.

Alternatively, you'll have to cast the argument to std::abs as int and the compiler should choose the right overload, but that's probably difficult with the Vector abstraction. Oh well, bugs / defects inside the software stacks are bad...

[mattkretz]

37d48ab should fix it for good now