Page MenuHomePhabricator

[AArch64][SVE] Allow C-style casts between fixed-size and scalable vectors
ClosedPublic

Authored by joechrisellis on Nov 11 2020, 7:12 AM.

Details

Summary

This patch allows C-style casting between fixed-size and scalable
vectors. This kind of cast was previously blocked by the compiler, but
it should be allowed.

Diff Detail

Unit TestsFailed

TimeTest
370 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
14,590 mslinux > libFuzzer.libFuzzer::only-some-bytes.test
Script: -- : 'RUN: at line 5'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address -I/mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/fuzzer -m64 -c -fno-sanitize=all -fsanitize=dataflow -mllvm -dfsan-fast-16-labels /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/fuzzer/../../lib/fuzzer/dataflow/DataFlow.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/only-some-bytes.test.tmp-DataFlow.o

Event Timeline

joechrisellis created this revision.Nov 11 2020, 7:12 AM
joechrisellis requested review of this revision.Nov 11 2020, 7:12 AM

Add tests for C-style casts to/from scalable/fixed float vector types.

fpetrogalli added inline comments.Nov 11 2020, 8:37 AM
clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.cpp
1 ↗(On Diff #304513)

Please add all cases supported, from 128 up to 2048 (doubling the size in each invocation).

16 ↗(On Diff #304513)

Maybe write a CPP macro to reduce the amount of typing in this test? The functions below they all look the same...

clang/test/Sema/aarch64-sve-explicit-casts.cpp
5 ↗(On Diff #304513)

Maybe use the same nomenclature of the ACLE specs, use SVE VLAT instead of just SVE.

7 ↗(On Diff #304513)

Maybe add a link to the specs, and the version? (like in 00bet6)

joechrisellis marked 4 inline comments as done.

Address @fpetrogalli's comments.

  • Add version of ACLE spec.
  • Use ACLE nomenclature.
  • Introduce preprocessor macro to reduce code size of aarch64-sve-explicit-casts.cpp.
  • Test more values for -msve-vector-bits in aarch64-sve-explicit-casts.cpp.
joechrisellis added inline comments.Nov 11 2020, 9:34 AM
clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.cpp
16 ↗(On Diff #304513)

Excellent idea, done!

@joechrisellis thanks for the patch Joe, I've added a few comments. I also noticed this only covers C++, do we want to test C as well?

clang/lib/Sema/SemaCast.cpp
2222–2227

This is a bit ambiguous, it'll allow bitcasting between things like a fixed predicate and any sizeless type which I don't think makes sense. I think it'll also allow bitcasting between any scalable and GNU vectors regardless of vector length, e.g.:

typedef int32_t int32x4_t __attribute__((vector_size(16)));

void foo() {
svint32_t s32;
int32x4_t g32;
g32 = (int32x4_t)s32;
s32 = (svint32_t)g32;
}

the above should only work when -msve-vector-bits=128 but will currently be allowed for any N.

clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.cpp
1 ↗(On Diff #304513)

C++ sema tests should be under clang/test/SemaCXX/

clang/test/Sema/aarch64-sve-explicit-casts.cpp
1–36 ↗(On Diff #304513)

clang/test/SemaCXX/sizeless-1.cpp already contains a test checking C-style casts between distinct sizeless types aren't supported, I'm not sure if adding this is necessary.

joechrisellis marked 2 inline comments as done.

Address @c-rhodes's comments.

  • Only allow casting between VLATs and VLSTs.
  • Add C test.
  • Move C++ test to the correct directory.
  • Remove superfluous test.
clang/lib/Sema/SemaCast.cpp
2222–2227

Ah, you're dead right. I think the next diff should fix this, but if there are any more inconsistencies/ambiguities I'll fix them too. :)

clang/test/Sema/aarch64-sve-explicit-casts.cpp
1–36 ↗(On Diff #304513)

ACK -- removed. :)

c-rhodes added inline comments.Nov 13 2020, 10:38 AM
clang/lib/Sema/SemaCast.cpp
2217–2218

nit: not sure we need to change this

2222–2227

Ah, you're dead right. I think the next diff should fix this, but if there are any more inconsistencies/ambiguities I'll fix them too. :)

I suppose it's outside of the scope of this ticket, but I think we'll still need to address support for explicit conversions between scalable vectors and GNU vectors like in the example I gave.

2763–2767

I think braces are recommended on the outer if as well, see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Although I suppose it could also be written as:

if ((SrcType->isVectorType() || DestType->isVectorType()) &&
    Self.isValidSveBitcast(SrcType, DestType)) {
  Kind = CK_BitCast;
  return;
}
clang/lib/Sema/SemaExpr.cpp
7209–7210

Missing a check for isSizelessBuiltinType

clang/test/SemaCXX/aarch64-sve-explicit-casts-fixed-size.cpp
23–69

nit: could simplify this a little further by wrapping the macro, e.g. (moving existing TESTCASE to CAST):

#define TESTCASE(ty1, ty2) \
    CAST(ty1, ty2)         \
    CAST(ty2, ty1)
25–26

nit: can just pass these as args void from##_to_##to(from a, to b) {\

clang/lib/Sema/SemaCast.cpp
2222

It's good to avoid use of pronouns such as "We" in comments like this, since a different reader might take a different interpretation of the word "We". Is "We" the software itself? Is it the company who wrote the condition? Better to rewrite it in a more direct way; in my suggestion it is written so that it's clearer the following statements are what allow it. Even better if you can include a reference to a spec indicating the background on why it is allowed.

2762

Another couple of uses of the word "we". Suggest taking Cullen's suggestion to combine the conditions and say:

"Allow bitcasting between compatible SVE vector types".

2763

I don't understand why this is an || rather than an &&, please can you clarify? I would have expected they must both be vectors.

clang/lib/Sema/SemaExpr.cpp
7200

Use of "we".

joechrisellis marked 7 inline comments as done.

Address @c-rhodes's and @peterwaller-arm's comments.

joechrisellis added inline comments.Nov 17 2020, 5:56 AM
clang/lib/Sema/SemaCast.cpp
2217–2218

I changed this because the code changes mean that the original comment is incomplete. I.e., we now allow for reinterpret casts VLAT <-> VLST, which is not captured by the comment above.

2763

The sizeless types defined by the ACLE are represented in Clang as BuiltinTypes rather than VectorTypes. The sized types are represented as VectorTypes. The only possibility for a valid C-style cast is when we're casting VLAT <-> VLST, which is if one of the two is a VectorType. :)

c-rhodes added inline comments.Nov 17 2020, 6:21 AM
clang/lib/Sema/SemaCast.cpp
2217–2218

I changed this because the code changes mean that the original comment is incomplete. I.e., we now allow for reinterpret casts VLAT <-> VLST, which is not captured by the comment above.

It's a minor point but the reason I raised it is we have to be conscious of the fact most people probably don't care about SVE and this is changing an existing comment to put it first and foremost.

joechrisellis added inline comments.Nov 17 2020, 6:25 AM
clang/lib/Sema/SemaCast.cpp
2217–2218

Ok, I see what you mean. Will revert this part.

Address @c-rhodes's comment regarding comment change.

c-rhodes accepted this revision.Nov 18 2020, 9:19 AM

LGTM

I've left one minor comment, if that suggestion works it should be fine to fix it before committing

clang/lib/Sema/SemaExpr.cpp
7210–7212

nit: I think you can just do:

if (!FirstType->isSizelessBuiltinType())
  return false;
This revision is now accepted and ready to land.Nov 18 2020, 9:19 AM

Avoid doing FirstType->getAs<BuiltinType>().

This revision was landed with ongoing or failed builds.Nov 19 2020, 3:18 AM
This revision was automatically updated to reflect the committed changes.