This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rsmith on Sep 17 2019, 1:34 PM.

Diff Detail

Event Timeline

rsmith created this revision.Sep 17 2019, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2019, 1:34 PM
rsmith updated this revision to Diff 224130.Oct 9 2019, 12:51 PM

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

rsmith updated this revision to Diff 224138.Oct 9 2019, 1:13 PM

Update documentation and release notes.

rsmith edited the summary of this revision. (Show Details)Oct 10 2019, 2:06 PM
rsmith added a reviewer: eli.friedman.
rsmith updated this revision to Diff 224473.Oct 10 2019, 2:07 PM

Test fixup.

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

efriedma added inline comments.
docs/ReleaseNotes.rst
84

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 marked an inline comment as done.Oct 29 2019, 6:21 PM
rsmith added inline comments.
docs/ReleaseNotes.rst
84

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 added inline comments.Oct 29 2019, 6:47 PM
docs/ReleaseNotes.rst
84

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 added inline comments.Oct 30 2019, 1:04 AM
docs/ReleaseNotes.rst
84

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 added inline comments.Oct 30 2019, 5:00 PM
docs/ReleaseNotes.rst
84

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

rsmith marked an inline comment as done.Oct 30 2019, 5:02 PM
rsmith added inline comments.
docs/ReleaseNotes.rst
84

@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 added inline comments.Nov 3 2019, 7:02 AM
docs/ReleaseNotes.rst
84

@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 updated this revision to Diff 238099.Jan 14 2020, 2:21 PM

Rebase and slightly expand release notes.

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

This revision is now accepted and ready to land.Jan 14 2020, 5:18 PM
This revision was automatically updated to reflect the committed changes.
hctim added a subscriber: hctim.Jan 20 2020, 4:39 PM

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.

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

@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 added a subscriber: dmajor.Jan 22 2020, 12:37 PM

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 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'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.

@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

@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.

@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.

@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.

@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 added a comment.Feb 3 2020, 6:20 PM

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 :)

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 reopened this revision.Apr 12 2022, 1:16 PM

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

This revision is now accepted and ready to land.Apr 12 2022, 1:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:16 PM

@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.

@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.