Fix __builtin_vectorelements tests with REQUIRES

[lawben]

Small fix for failing tests after merge of #69010. The tests need REQUIRES to ensure that the correct headers are available. I've also added a generic x86 build which does not need headers, so there is at least one run per test.

Side note: I'm still quite new to the LLVM test setup. a) Is this the correct way to do this and b) canI trigger the full tests before merging to main to avoid a second set of failed buildbots?

[llvmbot]

@llvm/pr-subscribers-clang

Author: Lawrence Benson (lawben)

Changes

Small fix for failing tests after merge of #69010. The tests need REQUIRES to ensure that the correct headers are available. I've also added a generic x86 build which does not need headers, so there is at least one run per test.

Side note: I'm still quite new to the LLVM test setup. a) Is this the correct way to do this and b) canI trigger the full tests before merging to main to avoid a second set of failed buildbots?

---

Full diff: https://github.com/llvm/llvm-project/pull/69582.diff

2 Files Affected:

• (modified) clang/test/CodeGen/builtin_vectorelements.c (+13-7)
• (modified) clang/test/SemaCXX/builtin_vectorelements.cpp (+3)

diff --git a/clang/test/CodeGen/builtin_vectorelements.c b/clang/test/CodeGen/builtin_vectorelements.c
index a825ab2b7273d52..06d9ee7e056a83e 100644
--- a/clang/test/CodeGen/builtin_vectorelements.c
+++ b/clang/test/CodeGen/builtin_vectorelements.c
@@ -1,10 +1,17 @@
-// RUN: %clang_cc1 -O1 -triple aarch64 -target-feature +neon %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK,NEON %s
-// RUN: %clang_cc1 -O1 -triple aarch64 -target-feature +sve  %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK,SVE  %s
-// RUN: %clang_cc1 -O1 -triple riscv64 -target-feature +v    %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK,RISCV  %s
+// RUN: %clang_cc1 -O1 -triple x86_64                        %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK       %s
 
-// Note that this does not make sense to check for x86 SIMD types, because
-// __m128i, __m256i, and __m512i do not specify the element type. There are no
-// "logical" number of elements in them.
+// REQUIRES: target=aarch64-{{.*}}
+// RUN: %clang_cc1 -O1 -triple aarch64 -target-feature +neon %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK,NEON  %s
+
+// REQUIRES: target=aarch64-{{.*}}
+// RUN: %clang_cc1 -O1 -triple aarch64 -target-feature +sve  %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK,SVE   %s
+
+// REQUIRES: target=riscv64{{.*}}
+// RUN: %clang_cc1 -O1 -triple riscv64 -target-feature +v    %s -emit-llvm -disable-llvm-passes -o - | FileCheck --check-prefixes=CHECK,RISCV %s
+
+/// Note that this does not make sense to check for x86 SIMD types, because
+/// __m128i, __m256i, and __m512i do not specify the element type. There are no
+/// "logical" number of elements in them.
 
 typedef int int1 __attribute__((vector_size(4)));
 typedef int int4 __attribute__((vector_size(16)));
@@ -56,7 +63,6 @@ int test_builtin_vectorelements_multiply_constant() {
   return __builtin_vectorelements(int16) * 2;
 }
 
-
 #if defined(__ARM_NEON)
 #include <arm_neon.h>
 
diff --git a/clang/test/SemaCXX/builtin_vectorelements.cpp b/clang/test/SemaCXX/builtin_vectorelements.cpp
index 423051def7f7c29..f40ba2a902cb5fc 100644
--- a/clang/test/SemaCXX/builtin_vectorelements.cpp
+++ b/clang/test/SemaCXX/builtin_vectorelements.cpp
@@ -1,3 +1,6 @@
+// RUN: %clang_cc1 -triple x86_64                       -std=c++20 -fsyntax-only -verify -disable-llvm-passes %s
+
+// REQUIRES: target=aarch64-{{.*}}
 // RUN: %clang_cc1 -triple aarch64 -target-feature +sve -std=c++20 -fsyntax-only -verify -disable-llvm-passes %s
 
 template <typename T>

[tbaederr]

a) Is this the correct way to do this

You don't need review for this I think. I can't review this patch. If it takes a while you should rather revert your original patch.

b) can I trigger the full tests before merging to main to avoid a second set of failed buildbots?

Not that I know of.

If you think this is the right way forward, you can merge this as-is, I don't know if this is correct. If this patch still doesn't fix the failing build bots, you should probably revert the original patch for the time being.

[nikic]

The correct check is REQUIRES: aarch64-registered-target, not REQUIRES: target=aarch64.

[lawben]

@nikic Sorry for the mess :/

I took the target= approach from here. Following the current buildbot status, this also does not seem to crash. Does this mean the rests are still incorrect and are not being executed at all? My local setup has all targets enabled, so it's hard for me to reproduce this locally.

If this is still incorrect, I'd kindly ask you to point out the correct way of doing this across all tests. I guess it would have REQUIRES: aarch64-registered-target for AArch64 and REQUIRES: riscv64-registered-target for RISCV?

[nikic]

@nikic Sorry for the mess :/

I took the target= approach from here. Following the current buildbot status, this also does not seem to crash. Does this mean the rests are still incorrect and are not being executed at all? My local setup has all targets enabled, so it's hard for me to reproduce this locally.

I expect the tests just don't run at all now.

If this is still incorrect, I'd kindly ask you to point out the correct way of doing this across all tests. I guess it would have REQUIRES: aarch64-registered-target for AArch64 and REQUIRES: riscv64-registered-target for RISCV?

The RISCV check would be REQUIRES: riscv-registered-target.