[clang][SVE] Don't warn on vector to sizeless builtin implicit conversion

[joechrisellis]

Summary

This commit prevents warnings from -Wconversion when a clang vector type
is implicitly converted to a sizeless builtin type -- for example, when
implicitly converting a fixed-predicate to a scalable predicate.

The code below:

1    #include <arm_sve.h>
2
3    #define N __ARM_FEATURE_SVE_BITS
4    #define FIXED_ATTR __attribute__((arm_sve_vector_bits (N)))
5    typedef svbool_t fixed_svbool_t FIXED_ATTR;
6
7    inline fixed_svbool_t foo(fixed_svbool_t p) {
8      return svnot_z(svptrue_b64(), p);
9    }

would previously raise this warning:

warning: implicit conversion turns vector to scalar: \
'fixed_svbool_t' (vector of 8 'unsigned char' values) to 'svbool_t' \
(aka '__SVBool_t') [-Wconversion]

Note that many cases of these implicit conversions were already
permitted because many functions inside arm_sve.h are spawned via
preprocessor macros, and the call to isInSystemMacro would cover us in
this case. This commit fixes the remaining cases.

[c-rhodes]

clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c

1–32

I wonder if we can just add -Wconversion to the existing Sema tests?

• clang/test/Sema/attr-arm-sve-vector-bits.c
• clang/test/SemaCXX/attr-arm-sve-vector-bits.cpp

[joechrisellis]

clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c

1–32

clang/test/Sema/attr-arm-sve-vector-bits.c contains a lot of // expected-error ... and such, so I'm reluctant to make changes to that because it seems a little weird to mix up expected errors and non-expected warnings.

I suppose we could cat this test into attr-arm-sve-vector-bits.cpp, though, if you would prefer? 🙂

[c-rhodes]

clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c

1–32

clang/test/Sema/attr-arm-sve-vector-bits.c contains a lot of // expected-error ... and such, so I'm reluctant to make changes to that because it seems a little weird to mix up expected errors and non-expected warnings.

I sort of see what you mean but unless there's a good reason (which I can't really think of) then I think it's preferential to introducing another test for this attribute. There's already a bunch of tests with this exact logic being tested here the only difference is -Wconversion is on.

To be honest I don't have strong feelings about it so it can stay as a separate test if you wish, but if it does I think this can be reduced to a single test for predicates and one extra test for a data vector like int8 or something.

[craig.topper]

Is this change specific to fixed vectors declared with arm_sve_vector_bits or any of the subclasses of VectorType? If it allows the others, how do we know for sure that there are enough bits in the scalable type for the fixed vector. I ask because RISCV is also using sizeless builtin types for our vectors as of D92715.

[joechrisellis]

Address comments.

• @c-rhodes: remove test; it is probably not necessary.
• @craig.topper: add better constraints for when we do/don't bail out.

[c-rhodes]

In D97053#2575791, @craig.topper wrote:

Is this change specific to fixed vectors declared with arm_sve_vector_bits or any of the subclasses of VectorType? If it allows the others, how do we know for sure that there are enough bits in the scalable type for the fixed vector. I ask because RISCV is also using sizeless builtin types for our vectors as of D92715.

Thanks for pointing out that out Craig, I wasn't aware RISCV is also using sizeless builtins now, I hadn't considered that.

clang/lib/Sema/SemaChecking.cpp

12055

I suppose we could tighten this further by replacing isSizelessBuiltinType with isVLSTBuiltinType

[joechrisellis]

Address @c-rhodes's comment.

• use isVLSTBuiltinType instead of isSizelessBuiltinType.

clang/lib/Sema/SemaChecking.cpp

12055

Good shout -- thanks. 😄

[c-rhodes]

LGTM thanks

clang/lib/Sema/SemaChecking.cpp

12054–12057

nit: can this warning be fixed before landing?

[joechrisellis]

Address linter comments.

clang/lib/Sema/SemaChecking.cpp

static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
   // Check implicit casts from Objective-C collection literals to specialized
   // collection types, e.g., NSArray<NSString *> *.
   if (auto *ArrayLiteral = dyn_cast<ObjCArrayLiteral>(E))
     checkObjCArrayLiteral(S, QualType(Target, 0), ArrayLiteral);
   else if (auto *DictionaryLiteral = dyn_cast<ObjCDictionaryLiteral>(E))
     checkObjCDictionaryLiteral(S, QualType(Target, 0), DictionaryLiteral);
 
   // Strip vector types.
   if (isa<VectorType>(Source)) {
+    // Converting from a vector to a sizeless builtin type is fine.
+    if (Target->isSizelessBuiltinType())
+      return;
     if (!isa<VectorType>(Target)) {
       if (S.SourceMgr.isInSystemMacro(CC))
         return;
       return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_vector_scalar);
     }
 
     // If the vector cast is cast between two vectors of the same size, it is
     // a bitcast, not a conversion.

clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c

+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve \
+// RUN: -msve-vector-bits=512 -Wconversion -fallow-half-arguments-and-returns \
+// RUN: -o /dev/null %s 2>&1 | FileCheck %s -allow-empty
+
+// CHECK-NOT: warning
+
+#include <arm_sve.h>
+
+#define N __ARM_FEATURE_SVE_BITS
+#define FIXED_ATTR __attribute__((arm_sve_vector_bits(N)))
+
+typedef svbool_t fixed_svbool_t FIXED_ATTR;
+
+fixed_svbool_t check_svnot(fixed_svbool_t p) {
+  return svnot_z(svptrue_b64(), p);
+}
+
+fixed_svbool_t check_svorr(fixed_svbool_t p1, fixed_svbool_t p2) {
+  return svorr_z(svptrue_b64(), p1, p2);
+}
+
+fixed_svbool_t check_svorn(fixed_svbool_t p1, fixed_svbool_t p2) {
+  return svorn_z(svptrue_b64(), p1, p2);
+}
+
+fixed_svbool_t check_svand(fixed_svbool_t p1, fixed_svbool_t p2) {
+  return svand_z(svptrue_b64(), p1, p2);
+}
+
+fixed_svbool_t check_svnand(fixed_svbool_t p1, fixed_svbool_t p2) {
+  return svnand_z(svptrue_b64(), p1, p2);
+}