PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

[rsmith]

Summary

See proposal on cfe-dev:
http://lists.llvm.org/pipermail/cfe-dev/2019-April/062030.html

[rsmith]

Remove special-casing of ARM NEON; let's wait for <arm_neon.h> to get fixed before landing this.

[rsmith]

Update documentation and release notes.

[rsmith]

Test fixup.

[rsmith]

Ping: <arm_neon.h> is fixed, this is now good to go.

[efriedma]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

And if you want to allow your code to build with both clang-9 and clang-10, you have to do version detection in your build scripts?

[rsmith]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

I guess you'd detect whether the compiler supports -flax-vector-conversions=all, and pass that if so, and otherwise pass -flax-vector-conversions. Well, either that or you fix your code to not rely on lax vector conversions between int and float vectors. If your code builds with GCC, you did that already (they never supported lax conversions between float and int vectors, as far as I can tell).

Do you have a preferred alternative?

[efriedma]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

All the alternatives are terrible in their own way:

1. This status quo, which breaks compatibility with previous versions of clang
2. We could make -flax-vector-conversions mean the same thing as earlier versions of clang. So the flag wouldn't have the same meaning as gcc's flag.
3. Some mix of the previous options: we could wait until there are one or two released versions that support -flax-vector-conversions=all , then change the meaning of -flax-vector-conversions. But I have no idea how we would decide on a timeline.

I ran into this issue recently updating our compiler. That said, the code in question was only using the implicit conversion in a couple places by accident, so it was easy enough to just fix the source.

[kristof.beyls]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

Maybe adding an entry in the release notes about this change could help with making option 1 slightly more palatable?
My guess is that option 1 is the right one for the long term (compatibility between gcc and clang so code is more portable between both compilers).

[efriedma]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

It probably makes sense to call out the behavior change to -flax-vector-conversions in the release notes, yes.

[rsmith]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

@kristof.beyls Are you looking for more changes to the release notes in addition to what's already in this change? If so, what would you like to see?

[kristof.beyls]

docs/ReleaseNotes.rst

84 ↗

(On Diff #224473)

@rsmith I'm afraid I reacted to the review comments above and completely missed you had already added an entry to the release notes! My apologies.
I think it might still take some time for someone getting a build error who then goes through the release notes to easily spot that it's the change to lax vector conversions that's making their build fail.
However, I can't think of a much better way to describe this in the release notes so that a developer would spot this more easily, unless we'd put a source code example of something that now fails by default that didn't before. Putting source code examples in the release notes for all changes might make the release notes too long/complex?

[rsmith]

Rebase and slightly expand release notes.

[rsmith]

Ping, I don't think I have any actionable feedback here and I'd like to get this landed before Clang 10 branches.

[efriedma]

LGTM

[hctim]

This change broke the sanitizer buildbots and was reverted here. Please see here for more information:

/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4232:7: error: no matching function for call to '_mm_cmpneq_ps'
  c = _mm_cmpneq_ps(V4x32{Poisoned<U4>(), 1, 2, 3}, V4x32{4, 5, Poisoned<U4>(), 6});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/xmmintrin.h:719:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V4x32' (vector of 4 'U4' values) to '__m128' (vector of 4 'float' values) for 1st argument
_mm_cmpneq_ps(__m128 __a, __m128 __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4238:7: error: no matching function for call to '_mm_cmpneq_ps'
  c = _mm_cmpneq_ps(V4x32{0, 1, 2, 3}, V4x32{4, 5, 6, 7});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/xmmintrin.h:719:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V4x32' (vector of 4 'U4' values) to '__m128' (vector of 4 'float' values) for 1st argument
_mm_cmpneq_ps(__m128 __a, __m128 __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4244:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{Poisoned<U8>(), 1}, V2x64{2, 3});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4246:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, 2}, V2x64{Poisoned<U8>(), 3});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4248:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, 2}, V2x64{3, 4});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4250:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, Poisoned<U8>()}, V2x64{2, Poisoned<U8>()});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4252:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, Poisoned<U8>()}, V2x64{1, Poisoned<U8>()});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4258:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{Poisoned<U8>(), 1}, V2x64{2, 3});
                               ^~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4260:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, 2}, V2x64{Poisoned<U8>(), 3});
                               ^~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4262:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, 2}, V2x64{3, 4});
                               ^~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4264:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, Poisoned<U8>()}, V2x64{2, Poisoned<U8>()});
                               ^~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4266:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, Poisoned<U8>()}, V2x64{1, Poisoned<U8>()});
                               ^~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4232:7: error: no matching function for call to '_mm_cmpneq_ps'
  c = _mm_cmpneq_ps(V4x32{Poisoned<U4>(), 1, 2, 3}, V4x32{4, 5, Poisoned<U4>(), 6});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/xmmintrin.h:719:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V4x32' (vector of 4 'U4' values) to '__m128' (vector of 4 'float' values) for 1st argument
_mm_cmpneq_ps(__m128 __a, __m128 __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4238:7: error: no matching function for call to '_mm_cmpneq_ps'
  c = _mm_cmpneq_ps(V4x32{0, 1, 2, 3}, V4x32{4, 5, 6, 7});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/xmmintrin.h:719:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V4x32' (vector of 4 'U4' values) to '__m128' (vector of 4 'float' values) for 1st argument
_mm_cmpneq_ps(__m128 __a, __m128 __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4244:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{Poisoned<U8>(), 1}, V2x64{2, 3});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4246:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, 2}, V2x64{Poisoned<U8>(), 3});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4248:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, 2}, V2x64{3, 4});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4250:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, Poisoned<U8>()}, V2x64{2, Poisoned<U8>()});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4252:7: error: no matching function for call to '_mm_cmpneq_sd'
  c = _mm_cmpneq_sd(V2x64{1, Poisoned<U8>()}, V2x64{1, Poisoned<U8>()});
      ^~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/clang_build/lib/clang/11.0.0/include/emmintrin.h:866:1: note: candidate function not viable: no known conversion from '(anonymous namespace)::V2x64' (vector of 2 'U8' values) to '__m128d' (vector of 2 'double' values) for 1st argument
_mm_cmpneq_sd(__m128d __a, __m128d __b)
^
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4258:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{Poisoned<U8>(), 1}, V2x64{2, 3});
                               ^~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4260:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, 2}, V2x64{Poisoned<U8>(), 3});
                               ^~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4262:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, 2}, V2x64{3, 4});
                               ^~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4264:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, Poisoned<U8>()}, V2x64{2, Poisoned<U8>()});
                               ^~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/msan/tests/msan_test.cpp:4266:32: error: cannot initialize a parameter of type '__attribute__((__vector_size__(2 * sizeof(double)))) double' (vector of 2 'double' values) with an rvalue of type '(anonymous namespace)::V2x64' (vector of 2 'U8' values)
  c = __builtin_ia32_ucomisdlt(V2x64{1, Poisoned<U8>()}, V2x64{1, Poisoned<U8>()});
                               ^~~~~~~~~~~~~~~~~~~~~~~~
12 errors generated.

[arphaman]

This also breaks the macOS SDK. @rsmith could you please disable this change for Darwin when you're recommitting this patch?

[steven_wu]

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Here is an example in macOS SDK (from /usr/include/simd/logic.h):

static inline SIMD_CFUNC simd_bool simd_any(simd_int2 x) {
  return (_mm_movemask_ps(simd_make_int4_undef(x)) & 0x3);
}

This change will cause problem for people using TOT clang with macOS SDK. Maybe we can start with warning or provide a toolchain overwrite for this?

[dmajor]

I noticed that this was merged to the 10.0 release branch. Should the merge be reverted while the dust settles on the trunk implementation?

FWIW this change breaks code in skia, as used in both Firefox and Chromium. I see that crbug.com/1042470 added the flag as a bandaid, and we'll probably have to do the same in Firefox. It's a bit frustrating to see a breaking change like this pop up late in the release cycle.

[rsmith]

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

(This was already reverted a couple of days ago.)

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I'll split out the new flag at least and re-land for Clang 10, while we figure out how to set the default.

[rsmith]

In D67678#1834957, @rsmith wrote:

I'll split out the new flag at least and re-land for Clang 10, while we figure out how to set the default.

Looks like there's nothing to re-land here: only the change of default was reverted, not splitting up the flag.

[steven_wu]

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

(This was already reverted a couple of days ago.)

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I'll split out the new flag at least and re-land for Clang 10, while we figure out how to set the default.

I am not sure if we ever have rules about compatibility against old SDKs. The earliest time we can fix SDK is the next major Xcode release, which might not
@dexonsmith

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

(This was already reverted a couple of days ago.)

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I'll split out the new flag at least and re-land for Clang 10, while we figure out how to set the default.

Thanks @rsmith!

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

[dexonsmith]

In D67678#1836628, @steven_wu wrote:

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

We can't comment on future releases.

That said, if this is urgent, let @arphaman know and we can try to qualify internally sometime in the next few months and we can report back what issues we find.

[hans]

I've cherry-picked the revert (edd4398f4cd33a305afbca76ac4e6590e9337f4d) to the 10.x branch in b079266dcb6d1ee6446d074ebd1d212a13ce0665.

[rsmith]

In D67678#1836668, @dexonsmith wrote:

In D67678#1836628, @steven_wu wrote:

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

We can't comment on future releases.

That said, if this is urgent, let @arphaman know and we can try to qualify internally sometime in the next few months and we can report back what issues we find.

Understood. The current situation is deeply unpalatable (the conversions we allow by default that this patch would have disabled are *evil* and have never been supported by GCC; it looks like Clang only ever allowed them as a bug, and we really need to turn them off by default for safety as much as for GCC compatibility). But this isn't a regression, at least, so I don't think this is especially urgent. (I was working on this because Agner Fog made an impassioned plea that we fix this, and I think our community owes Agner a favor or two...)

If there's no timeline to update the macOS SDK, then perhaps we could add a hack to Clang to allow these conversions only in limited contexts (the specific parts of the macOS SDK that are relying on them). Do you know how many such places there might be? If it's just a few functions, we could match against the function names, or depending on what SIMD_CFUNC expands to, perhaps we could match that. Failing that, we could set a platform-specific default or similar.

[dexonsmith]

In D67678#1836922, @rsmith wrote:

In D67678#1836668, @dexonsmith wrote:

In D67678#1836628, @steven_wu wrote:

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

We can't comment on future releases.

That said, if this is urgent, let @arphaman know and we can try to qualify internally sometime in the next few months and we can report back what issues we find.

Understood. The current situation is deeply unpalatable (the conversions we allow by default that this patch would have disabled are *evil* and have never been supported by GCC; it looks like Clang only ever allowed them as a bug, and we really need to turn them off by default for safety as much as for GCC compatibility). But this isn't a regression, at least, so I don't think this is especially urgent. (I was working on this because Agner Fog made an impassioned plea that we fix this, and I think our community owes Agner a favor or two...)

Yeah, it definitely seems like a good change.

If there's no timeline to update the macOS SDK, then perhaps we could add a hack to Clang to allow these conversions only in limited contexts (the specific parts of the macOS SDK that are relying on them). Do you know how many such places there might be? If it's just a few functions, we could match against the function names, or depending on what SIMD_CFUNC expands to, perhaps we could match that. Failing that, we could set a platform-specific default or similar.

A hack like this sounds pragmatic to me. An initial look suggests it's a small number of frameworks; I suspect many functions, but we could potentially match on partial file path or SIMD_CFUNC. We need a deeper audit across our internal stack to be sure, though. I don't think we'll have bandwidth for that until ~late February.

[rsmith]

In D67678#1836953, @dexonsmith wrote:

In D67678#1836922, @rsmith wrote:

If there's no timeline to update the macOS SDK, then perhaps we could add a hack to Clang to allow these conversions only in limited contexts (the specific parts of the macOS SDK that are relying on them). Do you know how many such places there might be? If it's just a few functions, we could match against the function names, or depending on what SIMD_CFUNC expands to, perhaps we could match that. Failing that, we could set a platform-specific default or similar.

A hack like this sounds pragmatic to me. An initial look suggests it's a small number of frameworks; I suspect many functions, but we could potentially match on partial file path or SIMD_CFUNC. We need a deeper audit across our internal stack to be sure, though. I don't think we'll have bandwidth for that until ~late February.

OK, that seems fine to me. I don't want this to miss another release if we can avoid it, but that should give plenty of time to iterate on something that keeps your SDK building :)

[rsmith]

In D67678#1856229, @rsmith wrote:

In D67678#1836953, @dexonsmith wrote:

In D67678#1836922, @rsmith wrote:

If there's no timeline to update the macOS SDK, then perhaps we could add a hack to Clang to allow these conversions only in limited contexts (the specific parts of the macOS SDK that are relying on them). Do you know how many such places there might be? If it's just a few functions, we could match against the function names, or depending on what SIMD_CFUNC expands to, perhaps we could match that. Failing that, we could set a platform-specific default or similar.

A hack like this sounds pragmatic to me. An initial look suggests it's a small number of frameworks; I suspect many functions, but we could potentially match on partial file path or SIMD_CFUNC. We need a deeper audit across our internal stack to be sure, though. I don't think we'll have bandwidth for that until ~late February.

OK, that seems fine to me. I don't want this to miss another release if we can avoid it, but that should give plenty of time to iterate on something that keeps your SDK building :)

@dexonsmith Ping, I'd like to get this done for the Clang 12 release; what do we need to do to make sure we're not causing problems for the macOS SDK?

[rsmith]

@dexonsmith @arphaman What do we need to do to get this re-landed?

[dexonsmith]

In D67678#3446526, @rsmith wrote:

@dexonsmith @arphaman What do we need to do to get this re-landed?

(I'm sorry I missed your ping two years ago :(... I was on an extended leave at the time and never ended up catching up on the email I missed.)

@arphaman is probably the best person to figure this out with you, but I'll start an internal thread to see if someone else can help too.

[nemanjai]

In D67678#3446526, @rsmith wrote:

@dexonsmith @arphaman What do we need to do to get this re-landed?

FWIW, on the PPC side, we are working on cleaning up altivec.h and plan to eventually make -fno-lax-vector-conversions the default.

include/clang/Basic/LangOptions.def

 LANGOPT(Digraphs          , 1, 0, "digraphs")
 BENIGN_LANGOPT(HexFloats  , 1, C99, "C99 hexadecimal float constants")
 LANGOPT(CXXOperatorNames  , 1, 0, "C++ operator name keywords")
 LANGOPT(AppleKext         , 1, 0, "Apple kext support")
 BENIGN_LANGOPT(PascalStrings, 1, 0, "Pascal string support")
 LANGOPT(WritableStrings   , 1, 0, "writable string support")
 LANGOPT(ConstStrings      , 1, 0, "const-qualified string support")
 ENUM_LANGOPT(LaxVectorConversions, LaxVectorConversionKind, 2,
-             LaxVectorConversionKind::All, "lax vector conversions")
+             LaxVectorConversionKind::Integer, "lax vector conversions")
+BENIGN_LANGOPT(LaxVectorConversionsSpecified, 1, 0, "whether lax vector conversions was specified")
 LANGOPT(AltiVec           , 1, 0, "AltiVec-style vector initializers")
 LANGOPT(ZVector           , 1, 0, "System z vector extensions")
 LANGOPT(Exceptions        , 1, 0, "exception handling")
 LANGOPT(ObjCExceptions    , 1, 0, "Objective-C exceptions")
 LANGOPT(CXXExceptions     , 1, 0, "C++ exceptions")
 LANGOPT(DWARFExceptions   , 1, 0, "dwarf exception handling")
 LANGOPT(SjLjExceptions    , 1, 0, "setjmp-longjump exception handling")
 LANGOPT(SEHExceptions     , 1, 0, "SEH .xdata exception handling")

lib/Basic/Targets/AArch64.h

public:
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool setCPU(const std::string &Name) override;
 
   bool useFP16ConversionIntrinsics() const override {
     return false;
   }
 
+  void adjust(LangOptions &Opts) override;
+
   void getTargetDefinesARMV81A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefinesARMV82A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefinesARMV83A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefinesARMV84A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;

lib/Basic/Targets/AArch64.cpp

bool AArch64TargetInfo::setCPU(const std::string &Name) {
   return isValidCPUName(Name);
 }
 
 void AArch64TargetInfo::fillValidCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   llvm::AArch64::fillValidCPUArchList(Values);
 }
 
+void AArch64TargetInfo::adjust(LangOptions &Opts) {
+  TargetInfo::adjust(Opts);
+
+  // FIXME: arm_neon.h makes use of lax vector conversions (including
+  // conversions between integer and floating-point type), so forcibly enable
+  // support for that language extension / bug whenever arm_neon.h would be
+  // usable.
+  if ((FPU & NeonMode) && !Opts.LaxVectorConversionsSpecified)
+    Opts.setLaxVectorConversions(LangOptions::LaxVectorConversionKind::All);
+}
+
 void AArch64TargetInfo::getTargetDefinesARMV81A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
 }
 
 void AArch64TargetInfo::getTargetDefinesARMV82A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   // Also include the ARMv8.1 defines

lib/Basic/Targets/ARM.h

public:
   bool setCPU(const std::string &Name) override;
 
   bool setFPMath(StringRef Name) override;
 
   bool useFP16ConversionIntrinsics() const override {
     return false;
   }
 
+  void adjust(LangOptions &Opts) override;
+
   void getTargetDefinesARMV81A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
 
   void getTargetDefinesARMV82A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 

lib/Basic/Targets/ARM.cpp

bool ARMTargetInfo::setFPMath(StringRef Name) {
   } else if (Name == "vfp" || Name == "vfp2" || Name == "vfp3" ||
              Name == "vfp4") {
     FPMath = FP_VFP;
     return true;
   }
   return false;
 }
 
+void ARMTargetInfo::adjust(LangOptions &Opts) {
+  TargetInfo::adjust(Opts);
+
+  // FIXME: arm_neon.h makes use of lax vector conversions (including
+  // conversions between integer and floating-point type), so forcibly enable
+  // support for that language extension / bug whenever arm_neon.h would be
+  // usable.
+  if ((FPU & NeonFPU) && !SoftFloat && ArchVersion >= 7 &&
+      !Opts.LaxVectorConversionsSpecified)
+    Opts.setLaxVectorConversions(LangOptions::LaxVectorConversionKind::All);
+}
+
 void ARMTargetInfo::getTargetDefinesARMV81A(const LangOptions &Opts,
                                             MacroBuilder &Builder) const {
   Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
 }
 
 void ARMTargetInfo::getTargetDefinesARMV82A(const LangOptions &Opts,
                                             MacroBuilder &Builder) const {
   // Also include the ARMv8.1-A defines

lib/Frontend/CompilerInvocation.cpp

#include "clang/Basic/LangStandards.def"
   Opts.PascalStrings = Args.hasArg(OPT_fpascal_strings);
   Opts.VtorDispMode = getLastArgIntValue(Args, OPT_vtordisp_mode_EQ, 1, Diags);
   Opts.Borland = Args.hasArg(OPT_fborland_extensions);
   Opts.WritableStrings = Args.hasArg(OPT_fwritable_strings);
   Opts.ConstStrings = Args.hasFlag(OPT_fconst_strings, OPT_fno_const_strings,
                                    Opts.ConstStrings);
   if (Arg *A = Args.getLastArg(OPT_flax_vector_conversions_EQ)) {
     using LaxKind = LangOptions::LaxVectorConversionKind;
+    Opts.LaxVectorConversionsSpecified = true;
     if (auto Kind = llvm::StringSwitch<Optional<LaxKind>>(A->getValue())
                         .Case("none", LaxKind::None)
                         .Case("integer", LaxKind::Integer)
                         .Case("all", LaxKind::All)
                         .Default(llvm::None))
       Opts.setLaxVectorConversions(*Kind);
     else
       Diags.Report(diag::err_drv_invalid_value)

test/Headers/altivec-header.c

-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -target-feature +altivec -ffreestanding -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple powerpc64-unknown-unknown -target-feature +altivec -ffreestanding -emit-llvm -flax-vector-conversions=none -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -target-feature +altivec -ffreestanding -emit-llvm -flax-vector-conversions=all -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple powerpc64-unknown-unknown -target-feature +altivec -ffreestanding -emit-llvm -x c++ -o - %s | FileCheck %s
 
 #include <altivec.h>
 
 // Verify that simply including <altivec.h> does not generate any code
 // (i.e. all inline routines in the header are marked "static")
 
 // CHECK: target triple = "powerpc64-
 // CHECK-NEXT: {{^$}}
 // CHECK-NEXT: {{llvm\..*}}

test/Headers/arm-neon-header.c

-// RUN: %clang_cc1 -triple thumbv7-apple-darwin10 -target-cpu cortex-a8 -fsyntax-only -Wvector-conversions -ffreestanding %s
+// RUN: %clang_cc1 -triple thumbv7-apple-darwin10 -target-cpu cortex-a8 -fsyntax-only -flax-vector-conversions=all -Wvector-conversions -ffreestanding %s
 // RUN: %clang_cc1 -triple thumbv7-apple-darwin10 -target-cpu cortex-a8 -fsyntax-only -flax-vector-conversions=none -ffreestanding %s
 // RUN: %clang_cc1 -x c++ -triple thumbv7-apple-darwin10 -target-cpu cortex-a8 -fsyntax-only -Wvector-conversions -ffreestanding %s
 
 // RUN: %clang -fsyntax-only               -ffreestanding --target=aarch64-none-eabi -march=armv8.2-a+fp16 -std=c89 -xc %s
 // RUN: %clang -fsyntax-only -Wall -Werror -ffreestanding --target=aarch64-none-eabi -march=armv8.2-a+fp16 -std=c99 -xc %s
 // RUN: %clang -fsyntax-only -Wall -Werror -ffreestanding --target=aarch64-none-eabi -march=armv8.2-a+fp16 -std=c11 -xc %s
 
 // RUN: %clang -fsyntax-only               -ffreestanding --target=aarch64_be-none-eabi -march=armv8.2-a+fp16 -std=c89 -xc %s

test/Headers/x86-intrinsics-headers.c

-// RUN: %clang_cc1 -fsyntax-only -ffreestanding %s
+// RUN: %clang_cc1 -fsyntax-only -ffreestanding -flax-vector-conversions=all %s
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -flax-vector-conversions=none %s
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -x c++ %s
 
 #if defined(i386) || defined(__x86_64__)
 
 #ifdef __SSE4_2__
 // nmmintrin forwards to smmintrin.
 #include <nmmintrin.h>
 #endif
 
 // immintrin includes all other intel intrinsic headers.
 #include <immintrin.h>
 
 #endif

test/Headers/x86intrin-2.c

-// RUN: %clang_cc1 -fsyntax-only -ffreestanding %s -verify
+// RUN: %clang_cc1 -fsyntax-only -ffreestanding -flax-vector-conversions=all %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -flax-vector-conversions=none %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -x c++ %s -verify
 // expected-no-diagnostics
 
 #if defined(i386) || defined(__x86_64__)
 
 // Include the metaheader that includes all x86 intrinsic headers.
 #include <x86intrin.h>

test/Headers/x86intrin.c

-// RUN: %clang_cc1 -fsyntax-only -ffreestanding %s -verify
+// RUN: %clang_cc1 -fsyntax-only -ffreestanding -flax-vector-conversions=all %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -flax-vector-conversions=none %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -x c++ %s -verify
 // expected-no-diagnostics
 
 #if defined(i386) || defined(__x86_64__)
 
 // Include the metaheader that includes all x86 intrinsic headers.
 #include <x86intrin.h>
 
 #endif

test/Sema/vector-assign.c

 // RUN: %clang_cc1 %s -verify -fsyntax-only -Wvector-conversion
 typedef unsigned int v2u __attribute__ ((vector_size (8)));
 typedef signed int v2s __attribute__ ((vector_size (8)));
 typedef signed int v1s __attribute__ ((vector_size (4)));
 typedef float v2f __attribute__ ((vector_size(8)));
 typedef signed short v4ss __attribute__ ((vector_size (8)));
 
 void test1() {
   v2s v1;
   v2u v2;
   v1s v3;
   v2f v4;
   v4ss v5;
   
   v1 = v2; // expected-warning {{incompatible vector types assigning to 'v2s' (vector of 2 'int' values) from 'v2u' (vector of 2 'unsigned int' values)}}
   v1 = v3; // expected-error {{assigning to 'v2s' (vector of 2 'int' values) from incompatible type 'v1s' (vector of 1 'int' value)}}
-  v1 = v4; // expected-warning {{incompatible vector types assigning to 'v2s' (vector of 2 'int' values) from 'v2f' (vector of 2 'float' values)}}
+  v1 = v4; // expected-error {{assigning to 'v2s' (vector of 2 'int' values) from incompatible type 'v2f' (vector of 2 'float' values)}}
   v1 = v5; // expected-warning {{incompatible vector types assigning to 'v2s' (vector of 2 'int' values) from 'v4ss' (vector of 4 'short' values)}}
   
   v2 = v1; // expected-warning {{incompatible vector types assigning to 'v2u' (vector of 2 'unsigned int' values) from 'v2s' (vector of 2 'int' values)}}
   v2 = v3; // expected-error {{assigning to 'v2u' (vector of 2 'unsigned int' values) from incompatible type 'v1s' (vector of 1 'int' value)}}
-  v2 = v4; // expected-warning {{incompatible vector types assigning to 'v2u' (vector of 2 'unsigned int' values) from 'v2f' (vector of 2 'float' values)}}
+  v2 = v4; // expected-error {{assigning to 'v2u' (vector of 2 'unsigned int' values) from incompatible type 'v2f' (vector of 2 'float' values)}}
   v2 = v5; // expected-warning {{incompatible vector types assigning to 'v2u' (vector of 2 'unsigned int' values) from 'v4ss' (vector of 4 'short' values)}}
   
   v3 = v1; // expected-error {{assigning to 'v1s' (vector of 1 'int' value) from incompatible type 'v2s' (vector of 2 'int' values)}}
   v3 = v2; // expected-error {{assigning to 'v1s' (vector of 1 'int' value) from incompatible type 'v2u' (vector of 2 'unsigned int' values)}}
   v3 = v4; // expected-error {{assigning to 'v1s' (vector of 1 'int' value) from incompatible type 'v2f' (vector of 2 'float' values)}}
   v3 = v5; // expected-error {{assigning to 'v1s' (vector of 1 'int' value) from incompatible type 'v4ss'}}
   
-  v4 = v1; // expected-warning {{incompatible vector types assigning to 'v2f' (vector of 2 'float' values) from 'v2s' (vector of 2 'int' values)}}
-  v4 = v2; // expected-warning {{incompatible vector types assigning to 'v2f' (vector of 2 'float' values) from 'v2u' (vector of 2 'unsigned int' values)}}
+  v4 = v1; // expected-error {{assigning to 'v2f' (vector of 2 'float' values) from incompatible type 'v2s' (vector of 2 'int' values)}}
+  v4 = v2; // expected-error {{assigning to 'v2f' (vector of 2 'float' values) from incompatible type 'v2u' (vector of 2 'unsigned int' values)}}
   v4 = v3; // expected-error {{assigning to 'v2f' (vector of 2 'float' values) from incompatible type 'v1s' (vector of 1 'int' value)}}
-  v4 = v5; // expected-warning {{incompatible vector types assigning to 'v2f' (vector of 2 'float' values) from 'v4ss' (vector of 4 'short' values)}}
+  v4 = v5; // expected-error {{assigning to 'v2f' (vector of 2 'float' values) from incompatible type 'v4ss' (vector of 4 'short' values)}}
   
   v5 = v1; // expected-warning {{incompatible vector types assigning to 'v4ss' (vector of 4 'short' values) from 'v2s' (vector of 2 'int' values)}}
   v5 = v2; // expected-warning {{incompatible vector types assigning to 'v4ss' (vector of 4 'short' values) from 'v2u' (vector of 2 'unsigned int' values)}}
   v5 = v3; // expected-error {{assigning to 'v4ss' (vector of 4 'short' values) from incompatible type 'v1s' (vector of 1 'int' value)}}
-  v5 = v4; // expected-warning {{incompatible vector types assigning to 'v4ss' (vector of 4 'short' values) from 'v2f'}}
+  v5 = v4; // expected-error {{assigning to 'v4ss' (vector of 4 'short' values) from incompatible type 'v2f'}}
 }
 
 // PR2263
 float test2(__attribute__((vector_size(16))) float a, int b) {
    return a[b];
 }
 
 // PR4838
 typedef long long __attribute__((__vector_size__(2 * sizeof(long long))))
 longlongvec;
 
 void test3a(longlongvec *); // expected-note{{passing argument to parameter here}}
 void test3(const unsigned *src) {
   test3a(src);  // expected-warning {{incompatible pointer types passing 'const unsigned int *' to parameter of type 'longlongvec *'}}
 }

test/Sema/vector-cast.c

-// RUN: %clang_cc1 -fsyntax-only %s -verify -Wvector-conversion
+// RUN: %clang_cc1 -fsyntax-only %s -verify -Wvector-conversion -flax-vector-conversions=all
 
 typedef long long t1 __attribute__ ((vector_size (8)));
 typedef char t2 __attribute__ ((vector_size (16)));
 typedef float t3 __attribute__ ((vector_size (16)));
 typedef short s2 __attribute__ ((vector_size(4)));
 
 typedef enum { Evalue = 0x10000 } E;
 

test/Sema/vector-ops.c

-// RUN: %clang_cc1 %s -verify -fsyntax-only -Wvector-conversion -triple x86_64-apple-darwin10
+// RUN: %clang_cc1 %s -verify -fsyntax-only -Wvector-conversion -triple x86_64-apple-darwin10 -flax-vector-conversions=all
 typedef unsigned int v2u __attribute__ ((vector_size (8)));
 typedef int v2s __attribute__ ((vector_size (8)));
 typedef float v2f __attribute__ ((vector_size(8)));
 
 void test1(v2u v2ua, v2s v2sa, v2f v2fa) {
   // Bitwise binary operators
   (void)(v2ua & v2ua);
   (void)(v2fa & v2fa); // expected-error{{invalid operands to binary expression}}