This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by joechrisellis on Feb 19 2021, 7:51 AM.

Details

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.

Diff Detail

Event Timeline

joechrisellis created this revision.Feb 19 2021, 7:51 AM
joechrisellis requested review of this revision.Feb 19 2021, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 7:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
c-rhodes added inline comments.Feb 19 2021, 8:33 AM
clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c
1–32 ↗(On Diff #324984)

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 added inline comments.Feb 19 2021, 9:25 AM
clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c
1–32 ↗(On Diff #324984)

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 added inline comments.Feb 19 2021, 9:58 AM
clang/test/Sema/aarch64-fixed-vector-to-scalable-implicit-conversion.c
1–32 ↗(On Diff #324984)

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.

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.

Address comments.

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

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 marked an inline comment as done.

Address @c-rhodes's comment.

  • use isVLSTBuiltinType instead of isSizelessBuiltinType.
clang/lib/Sema/SemaChecking.cpp
12055

Good shout -- thanks. 😄

c-rhodes accepted this revision.Feb 22 2021, 10:04 AM

LGTM thanks

clang/lib/Sema/SemaChecking.cpp
12054

nit: can this warning be fixed before landing?

This revision is now accepted and ready to land.Feb 22 2021, 10:04 AM
joechrisellis marked an inline comment as done.

Address linter comments.

This revision was landed with ongoing or failed builds.Feb 23 2021, 5:41 AM
This revision was automatically updated to reflect the committed changes.