Page MenuHomePhabricator

Fix GCC intrinsics "round_mask" names
Needs RevisionPublic

Authored by GuillaumeGomez on Jun 9 2022, 8:53 AM.

Details

Diff Detail

Event Timeline

GuillaumeGomez created this revision.Jun 9 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 8:53 AM
GuillaumeGomez requested review of this revision.Jun 9 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 8:53 AM

We encountered this issue while working on https://github.com/rust-lang/rustc_codegen_gcc (the GCC backend of the Rust compiler).

I used https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/gcc.target/i386/avx-1.c as reference for the renaming.

GCCBuiltin here does not mean it needs to match the real gcc compiler. I think is a leftover from when LLVM was a backend for gcc before clang existed. The names here need to match the names in clang/include/clang/Basic/BuiltinsX86.def. Does this patch pass make check-clang?

It doesn't pass the check-clang (putting the output just below). I'll fix it. Also, the files are still maintained and got very recent updates, hence why I think it was worth it to send this patch.

Command Output (stderr):
--
/home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512dq-builtins.c:937:10: error: cannot compile this builtin function yet
  return _mm_range_round_sd(__A, __B, 4, 8); 
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include/avx512dqintrin.h:886:13: note: expanded from macro '_mm_range_round_sd'
  ((__m128d)__builtin_ia32_rangesd128_round_mask((__v2df)(__m128d)(A), \
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512dq-builtins.c

--

********************
FAIL: Clang :: CodeGen/X86/avx512er-builtins.c (3043 of 15508)
******************** TEST 'Clang :: CodeGen/X86/avx512er-builtins.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/imperio/rust/llvm-project/build/bin/clang -cc1 -internal-isystem /home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -flax-vector-conversions=none -ffreestanding /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512er-builtins.c -triple=x86_64-apple-darwin -target-feature +avx512f -target-feature +avx512er -emit-llvm -o - -Wall -Werror | /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512er-builtins.c
--
Exit Code: 2

Command Output (stderr):
--
/home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512er-builtins.c:81:10: error: cannot compile this builtin function yet
  return _mm_rsqrt28_round_ss(a, b, _MM_FROUND_NO_EXC);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include/avx512erintrin.h:115:12: note: expanded from macro '_mm_rsqrt28_round_ss'
  ((__m128)__builtin_ia32_rsqrt28ss_round_mask((__v4sf)(__m128)(A), \
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512er-builtins.c

--

********************
FAIL: Clang :: CodeGen/X86/avx512fp16-builtins.c (3047 of 15508)
******************** TEST 'Clang :: CodeGen/X86/avx512fp16-builtins.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/imperio/rust/llvm-project/build/bin/clang -cc1 -internal-isystem /home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -ffreestanding -flax-vector-conversions=none /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512fp16-builtins.c -triple=x86_64-unknown-unknown -target-feature +avx512fp16 -emit-llvm -o - -Wall -Werror | /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512fp16-builtins.c
--
Exit Code: 2

Command Output (stderr):
--
/home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512fp16-builtins.c:729:10: error: cannot compile this builtin function yet
  return _mm_add_round_sh(__A, __B, _MM_FROUND_NO_EXC | _MM_FROUND_TO_ZERO);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include/avx512fp16intrin.h:592:13: note: expanded from macro '_mm_add_round_sh'
  ((__m128h)__builtin_ia32_addsh_round_mask(                                   \
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512fp16-builtins.c

--

********************
FAIL: Clang :: CodeGen/X86/avx512f-builtins.c (3048 of 15508)
******************** TEST 'Clang :: CodeGen/X86/avx512f-builtins.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/imperio/rust/llvm-project/build/bin/clang -cc1 -internal-isystem /home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -flax-vector-conversions=none -ffreestanding /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512f-builtins.c -triple=x86_64-apple-darwin -target-feature +avx512f -emit-llvm -o - -Wall -Werror -Wsign-conversion | /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512f-builtins.c
: 'RUN: at line 2';   /home/imperio/rust/llvm-project/build/bin/clang -cc1 -internal-isystem /home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -flax-vector-conversions=none -fms-extensions -fms-compatibility -ffreestanding /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512f-builtins.c -triple=x86_64-windows-msvc -target-feature +avx512f -emit-llvm -o - -Wall -Werror -Wsign-conversion | /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512f-builtins.c
--
Exit Code: 2

Command Output (stderr):
--
/home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512f-builtins.c:3143:10: error: cannot compile this builtin function yet
  return _mm_add_round_ss(__A,__B,_MM_FROUND_TO_ZERO | _MM_FROUND_NO_EXC);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/imperio/rust/llvm-project/build/lib/clang/15.0.0/include/avx512fintrin.h:1907:12: note: expanded from macro '_mm_add_round_ss'
  ((__m128)__builtin_ia32_addss_round_mask((__v4sf)(__m128)(A), \
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/imperio/rust/llvm-project/build/bin/FileCheck /home/imperio/rust/llvm-project/clang/test/CodeGen/X86/avx512f-builtins.c

--

********************
********************
Failed Tests (4):
  Clang :: CodeGen/X86/avx512dq-builtins.c
  Clang :: CodeGen/X86/avx512er-builtins.c
  Clang :: CodeGen/X86/avx512f-builtins.c
  Clang :: CodeGen/X86/avx512fp16-builtins.c


Testing Time: 204.82s
  Skipped          :     5
  Unsupported      :   126
  Passed           : 30541
  Expectedly Failed:    28
  Failed           :     4

I think I wasn't very clear. I only meant that the name "GCCBuiltin" is a leftover. At this point it means "ClangBuiltin". Maybe we should rename that.

Oh I see. Then should I close this patch and open another one for just renaming GCCBuiltin into ClangBuiltin?

Fix GCC intrinsics "round_mask" names

Update all _round_mask and formatting.

Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 2:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Just to have a vague idea of what this change would look like, I pushed it. It would make our lives much simpler if we could rely on GCCBuiltin (aka ClangBuiltin) though.

As you suggested, I opened https://reviews.llvm.org/D127460 to rename GCCBuiltin into ClangBuiltin to clarify the confusion.

pengfei requested changes to this revision.Jun 9 2022, 11:37 PM

We had a discussion about the builtins between GCC and Clang on D109658 last year. We tried to make sure the same builtins have the same arguments and behavior. There's still a big gap among others.
We don't pursue all the builtins are identical for two reasons:

  • Builtins are compiler private interfaces which are not supposed to be used directly and no guaranteed across compilers. User should use intrinsics instead;
  • Builtins are not 1:1 mapped between GCC and Clang, e.g., Clang prefer to use select rather than adding new mask builtins like GCC;

And changing the existing builtins will fall into a paradox: if we change it because there're quite a lot of user scenarios, then we cannot change it because the change will break user scenarios. And vice versa.
So I suggest to not change it, if you have to use builtins, you can use macro to distinguish each other.

This revision now requires changes to proceed.Jun 9 2022, 11:37 PM