llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.05k stars 11.98k forks source link

do not allow implicit conversions between unrelated vector types #17538

Open llvmbot opened 11 years ago

llvmbot commented 11 years ago
Bugzilla Link 17164
Version trunk
OS All
Depends On llvm/llvm-project#42682 llvm/llvm-project#42686
Reporter LLVM Bugzilla Contributor
CC @Axel-Naumann,@DougGregor,@RKSimon,@riccibruno,@zygoloid,@rotateright

Extended Description

Please see http://stackoverflow.com/questions/9906663/clang-error-ambiguous-conversion-for-static-cast/18693225#18693225

Similar to bug 7934, but still present in clang 3.4

Same error occurs when compiling vector class library with clang: http://agner.org/optimize/#vectorclass

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 2 years ago

mentioned in issue llvm/llvm-project#42686

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 2 years ago

mentioned in issue llvm/llvm-project#42682

llvmbot commented 2 years ago

mentioned in issue llvm/llvm-bugzilla-archive#25557

llvmbot commented 5 years ago

Thank you Richard for finally fixing this old problem.

Is there a preprocessing #define that can be used for checking the status of -flax-vector-conversions?

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 5 years ago

As of r371805, clang now supports -flax-vector-conversions={all,integer,none} to control whether you get Clang's old "anything goes" behavior, or the integer-only conversions described in comment#12, or no lax conversions.

For now, the default is unchanged, because clang's intrinsics headers break if lax vector conversions are disabled :( That's the next thing to fix.

llvmbot commented 5 years ago

This bug is now more than five years old. Is nothing happening? What can I do to make somebody take action???

llvmbot commented 6 years ago

Bug llvm/llvm-bugzilla-archive#25557 has been marked as a duplicate of this bug.

llvmbot commented 7 years ago

At last a sensible reaction after 3½ years! Thank you Richard.

We still perform implicit bitcasts in clang trunk. Perhaps Apple has a local patch to change the default for this flag.

I'm curious about this:

We need to [...] allow implicit conversions between __m128i as two 64-bit integers, or four 32-bit integers, or eight 16-bit integers or sixteen 8-bit integers, signed or unsigned.

We can certainly add a third mode to -flax-vector-conversions to disallow only int <-> float conversions, but allowing the above conversions to be performed implicitly also seems pretty broken. Do you believe that large amounts of code are relying on these conversions?

The Gnu, Intel, and Microsoft compilers are all compatible with each other. They distinguish between only three types of 128 bit vectors: __m128 = 4 * float __m128d = 2 * double __m128i = 16 * int8_t, 8 * int16_t, 4 * int32_t, 2 * int64_t, as well as unsigned versions. Analogously with __m256i and __m512i.

The C/C++ intrinsic functions that come with these compilers all rely on this. There are now thousands of intrinsic functions so we definitely don't want to fork these.

Compatibility between compilers is very important. Nobody wants to make different versions of their code for different compilers and platforms. The costs of testing and maintaining multiple versions of the code are simply too high. So the answer to your question is YES. All existing code that is not developed specifically for Clang relies on the intrinsic functions where the intrinsic vector types are defined as above. (My own vector class library, which is used by many programmers, works with all four compilers with a preprocessing hack for Clang).

We definitely need a command line switch that makes Clang compatible with the other compilers and a preprocessing macro that can be used for testing if this switch is on.

Of course we also need compatibility between Clang for different platforms. Can somebody please investigate what has happened with Apple Clang with regard to these vector types? I am not a Mac expert.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 7 years ago

We still perform implicit bitcasts in clang trunk. Perhaps Apple has a local patch to change the default for this flag.

I'm curious about this:

We need to [...] allow implicit conversions between __m128i as two 64-bit integers, or four 32-bit integers, or eight 16-bit integers or sixteen 8-bit integers, signed or unsigned.

We can certainly add a third mode to -flax-vector-conversions to disallow only int <-> float conversions, but allowing the above conversions to be performed implicitly also seems pretty broken. Do you believe that large amounts of code are relying on these conversions?

llvmbot commented 7 years ago

Has this bug finally been fixed? The behavior seems to have changed on Clang for Apple/Mac version 6.02.

Can anybody confirm this? Has it been fixed in Clang for other platforms too, and if so - which version?

llvmbot commented 8 years ago

I have changed the version to "Trunk" and "All platforms". The Mac people have done nothing on this bug for three years.

Proposed solution: Make a command line parameter to switch between two options:

  1. Vector register intrinsic types are compatible with all other compilers, i.e. __m128 != __m128i != __m128d, __m256 != __m256i != __m256d, __m512 != __m512i != __m512d.
  2. Compatible with code written for earlier versions of Clang, i.e. __m128 = __m128i = __m128d, __m256 = __m256i = __m256d, __m512 = __m512i = __m512d.

Preferably, option 1 should be the default.

Make a preprocessing #define that tells if option 1 is activated. This is necessary for C/C++ code that circumvents the bug with #if or #ifdef directives.

llvmbot commented 8 years ago

Why is this bug still not fixed in llvm/clang version 3.9.0? Is there a command line option that solves the problem? (see my comment 2013-09-11)

llvmbot commented 10 years ago

This problem is still not fixed in version 3.5. Please fix it for the sake of compatibility with Gcc, Intel and MS compilers and to avoid inadvertent bit-casts.

I need to know which version of Clang will fix it so that I can make a preprocessing #if with a work-around. This is important for my vector class library, which already has many users (http://www.agner.org/optimize/#vectorclass)

llvmbot commented 11 years ago

Correction: -fno-lax-vector-conversions does solve this specific problem but causes a lot of other problems because it seems to distinguish between different subtypes of __m128i with different integer sizes. This is incompatible with other compilers.

We need to turn off the implicit conversion between __m128, __m128i, __m128d and between __m256, __m256i, __m256d, but allow implicit conversions between __m128i as two 64-bit integers, or four 32-bit integers, or eight 16-bit integers or sixteen 8-bit integers, signed or unsigned.

llvmbot commented 11 years ago

It works with the option -fno-lax-vector-conversions

This option prevents implicit conversions between __m128, __m128i, __m128d and between __m256, __m256i, __m256d.

I think -flax-vector-conversions should be OFF by default

llvmbot commented 11 years ago

Can you turn off the implicit conversions between intrinsic vector types? This would be the most straightforward solution.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 11 years ago

Thanks for the investigation!

Yes, these implicit conversions are very troubling, both because they are not gcc-compatible, and also because they silently do something quite surprising:

typedef int m128i attribute((vector_size(16))); typedef float m128 attribute((vector_size(16))); m128 x; m128i y = x;

... bitcasts the float vector to an int vector.

llvmbot commented 11 years ago

Thank you for a prompt reply. You are right, the example on stackoverflow also fails on g++. Here is the reduced testcase from my vector class library:

#ifdef __GNUC__
    #include <x86intrin.h> 
#else
    #include <immintrin.h>
#endif

class Vec4i  {
protected:
    __m128i xmm;
public:
    Vec4i() {};
    Vec4i(__m128i const & x) {
        xmm = x;
    };
};

class Vec4fb {
protected:
    __m128 xmm;
public:
    Vec4fb() {};
    operator __m128() const { 
        return xmm;
    }
    operator Vec4i() const {
        return _mm_castps_si128(xmm);
    }
};

int main() {
    Vec4fb a;
    Vec4i b = Vec4i(a);
    return 0;
}
/*
clang_problem.cpp:36:15: error: ambiguous conversion for functional-style cast from 'Vec4fb' to 'Vec4i'
    Vec4i b = Vec4i(a);
              ^~~~~~~
clang_problem.cpp:10:7: note: candidate is the implicit copy constructor
class Vec4i  {
      ^
clang_problem.cpp:15:5: note: candidate constructor
    Vec4i(__m128i const & x) {
    ^
1 error generated.
*/

This test case works on Gnu, MS and Intel compilers. I can see the reason why it fails on clang: clang allows implicit conversions between __m128, __m128i and __m128d, the other compilers do not.

My code is a universal vector class library which would be very useful to clang users so I hope you can find a solution. I think compilers should be compatible as far as possible, and I would hate to make a special case in the code for clang

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 11 years ago

The testcase in the original post on stackoverflow is ill-formed (and g++ and EDG also reject it. It reduces to this:

struct B {                    
  B(const B &);
  B(const unsigned short &);
};

struct A {                         
  operator B() const;
  operator int() const;
};

B process() {                                           
  return static_cast<B>(A());
}

... which is ambiguous, because the conversion using A::operator B() and the conversion using A::operator int() are incomparable: [over.ics.rank] (13.3.3.2)/3 says "User-defined conversion sequence U1 is a better conversion sequence than another user-defined conversion sequence U2 if they contain the same user-defined conversion function and [...]".

Can you provide a testcase from your vector class library?