Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[X86] Change target of __builtin_ia32_cmp[p|s][s|d] from avx into sse/sse2 #67410

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

FreddyLeaf
Copy link
Contributor

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics labels Sep 26, 2023
@FreddyLeaf
Copy link
Contributor Author

This PR is to align with icc: https://godbolt.org/z/EzbfzTrzr We can modify intrinsic guide if required.

intel.com/SDM shows:

CMPSD xmm1, xmm2/m64, imm8
SSE2: Compare low double-precision floating-point value in xmm2/m64 and xmm1 using bits 2:0 of imm8 as comparison predicate
Intel C/C++ Compiler Intrinsic Equivalent
(V)CMPSD _m128d _mm_cmp_sd(_m128d a, __m128d b, const int imm)

@github-actions
Copy link

github-actions bot commented Sep 26, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 7675f541f75baa20e8ec007cd625a837e89fc01f 96ca4d04529e70d8ec75e7f44ddd35f072fe4d10 -- clang/test/CodeGen/X86/cmp-avx-builtins-error.c clang/lib/Headers/avxintrin.h clang/lib/Headers/emmintrin.h clang/lib/Headers/xmmintrin.h clang/lib/Sema/SemaChecking.cpp clang/test/CodeGen/X86/avx-builtins.c clang/test/CodeGen/X86/sse-builtins.c clang/test/CodeGen/X86/sse2-builtins.c clang/test/CodeGen/target-features-error-2.c clang/test/Sema/builtins-x86.c
View the diff from clang-format here.
diff --git a/clang/lib/Headers/xmmintrin.h b/clang/lib/Headers/xmmintrin.h
index 73b1ab4bb4a3..a2ad35a7fa16 100644
--- a/clang/lib/Headers/xmmintrin.h
+++ b/clang/lib/Headers/xmmintrin.h
@@ -2937,14 +2937,14 @@ _mm_movemask_ps(__m128 __a)
 }
 
 /* Compare */
-#define _CMP_EQ_OQ    0x00 /* Equal (ordered, non-signaling)  */
-#define _CMP_LT_OS    0x01 /* Less-than (ordered, signaling)  */
-#define _CMP_LE_OS    0x02 /* Less-than-or-equal (ordered, signaling)  */
-#define _CMP_UNORD_Q  0x03 /* Unordered (non-signaling)  */
-#define _CMP_NEQ_UQ   0x04 /* Not-equal (unordered, non-signaling)  */
-#define _CMP_NLT_US   0x05 /* Not-less-than (unordered, signaling)  */
-#define _CMP_NLE_US   0x06 /* Not-less-than-or-equal (unordered, signaling)  */
-#define _CMP_ORD_Q    0x07 /* Ordered (non-signaling)   */
+#define _CMP_EQ_OQ 0x00   /* Equal (ordered, non-signaling)  */
+#define _CMP_LT_OS 0x01   /* Less-than (ordered, signaling)  */
+#define _CMP_LE_OS 0x02   /* Less-than-or-equal (ordered, signaling)  */
+#define _CMP_UNORD_Q 0x03 /* Unordered (non-signaling)  */
+#define _CMP_NEQ_UQ 0x04  /* Not-equal (unordered, non-signaling)  */
+#define _CMP_NLT_US 0x05  /* Not-less-than (unordered, signaling)  */
+#define _CMP_NLE_US 0x06  /* Not-less-than-or-equal (unordered, signaling)  */
+#define _CMP_ORD_Q 0x07   /* Ordered (non-signaling)   */
 
 /// Compares each of the corresponding values of two 128-bit vectors of
 ///    [4 x float], using the operation specified by the immediate integer

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong objections, but I think the documentation needs to be tweaked to better explain SSE vs AVX handling.

clang/lib/Headers/emmintrin.h Outdated Show resolved Hide resolved
clang/lib/Headers/emmintrin.h Show resolved Hide resolved
@FreddyLeaf
Copy link
Contributor Author

FreddyLeaf commented Sep 26, 2023

No strong objections, but I think the documentation needs to be tweaked to better explain SSE vs AVX handling.

For intrinsic guide, I have an idea to show two results for the _mm_cmp_sd, one's CPUID is SSE2, the other one's is AVX.

Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still missing SSE vs AVX sema checking test coverage?

clang/lib/Headers/xmmintrin.h Outdated Show resolved Hide resolved
@FreddyLeaf
Copy link
Contributor Author

Can we land this patch now?

@FreddyLeaf
Copy link
Contributor Author

Are we still missing SSE vs AVX sema checking test coverage?

yes, added in 96ca4d0

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - cheers

@FreddyLeaf FreddyLeaf merged commit ccd5b8d into llvm:main Sep 27, 2023
1 of 2 checks passed
@FreddyLeaf FreddyLeaf deleted the cmp_sse branch September 27, 2023 13:24
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
@bgra8
Copy link
Contributor

bgra8 commented Oct 4, 2023

@FreddyLeaf functions with __attribute__((target("avx"))) trigger the newly added warning.

Code like this no longer builds and creates a lot of fallout for us (google internal code):

#include <immintrin.h>

__attribute__((target("avx"))) void test(__m128 a, __m128 b) {
  _mm_cmp_ps(a, b, 14);
}

Compiler output:

test.cc:4:3: error: argument value 14 is outside the valid range [0, 7] [-Wargument-outside-range]
    4 |   _mm_cmp_ps(a, b, 14);

Passing -mavx in the compilation command line makes the build green but the attribute target("avx") should be equivalent.

Please revert

bgra8 pushed a commit that referenced this pull request Oct 5, 2023
…into sse/sse2 (#67410)"

Does not respect `__attribute__((target("avx"))`.

This reverts commit ccd5b8d.
@FreddyLeaf FreddyLeaf restored the cmp_sse branch February 2, 2024 03:20
FreddyLeaf added a commit that referenced this pull request Mar 9, 2024
…into sse/sse2/avx (#84136)

This patch relands #67410 and fixes the cmpfail below:
#include <immintrin.h>
__attribute__((target("avx"))) void test(__m128 a, __m128 b) {
  _mm_cmp_ps(a, b, 14);
}

According to Intel SDM, SSE/SSE2 instructions cmp[p|s][s|d] are
supported when imm8 is in range of [0, 7]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants