Page MenuHomePhabricator

[Clang] Allow "ext_vector_type" applied to Booleans
Needs ReviewPublic

Authored by simoll on Oct 6 2020, 8:27 AM.

Details

Summary

This is the ext_vector_type alternative to D81083.

This patch extends Clang to allow 'bool' as a valid vector element type (attribute ext_vector_type) in C/C++.

This is intended as the canonical type for SIMD masks and facilitates clean vector intrinsic declarations.
Vectors of i1 are supported on IR level and below down to many SIMD ISAs, such as AVX512, ARM SVE (fixed vector length) and the VE target (NEC SX-Aurora TSUBASA).

The RFC on cfe-dev: https://lists.llvm.org/pipermail/cfe-dev/2020-May/065434.html

Diff Detail

Unit TestsFailed

TimeTest
390 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

Event Timeline

simoll created this revision.Oct 6 2020, 8:27 AM
rsmith added inline comments.Oct 30 2020, 12:04 PM
clang/docs/LanguageExtensions.rst
466

Comment talks about bool8 but we defined the type bool4.

488–489

Given that we support ExtInt(n) integer types for arbitrary n, I don't think this is clear. Perhaps "is the same as that of the lowest-rank standard integer type of at least the specified width" or some less wordy way of saying the same thing?

What happens if you ask for more vector elements than the bit width of unsigned long long?

Is the rule "The size and alignment are both the number of bits rounded up to the next power of two, but the alignment is at most the maximum vector alignment of the target." ?

clang/lib/AST/ASTContext.cpp
1932

Nit: please ensure that comments are formatted as full sentences (beginning with a capital letter and ending with a period) here and throughout the patch.

clang/lib/AST/TypePrinter.cpp
654 ↗(On Diff #296470)

If it's an ext_vector_type, we shouldn't be printing it as __vector_size__. ExtVectorBoolean isn't really a special case here; we should be distinguishing ExtVectorType from regular VectorType in general.

clang/lib/CodeGen/CGExpr.cpp
1690–1727
1770–1775

Consider moving isExtVectorBoolean() to Type::isExtVectorBoolType() or similar.

1835–1836
clang/lib/CodeGen/CGExprScalar.cpp
2035

"Otw"?

2039–2041

I don't think we should represent widening a vector of booleans to a mask vector of 0/-1 integers as a CK_BitCast. CK_BitCast is intended for cases where the bit-pattern is reinterpreted, not where it's modified.

Can we add a new cast kind for this instead?

clang/lib/Sema/SemaExpr.cpp
7228–7230

Please use && not & unless you intend a bitwise operation.

majnemer added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
2790

The code as written seems to be 'char' type, not 'unsigned char' type.

clang/lib/CodeGen/CGExpr.cpp
2080–2081

if (auto *IRStoreTy = dyn_cast<llvm::IntegerType>(Vec->getType()))

clang/lib/CodeGen/CodeGenTypes.cpp
97

dyn_cast -> cast

simoll updated this revision to Diff 302559.Tue, Nov 3, 5:54 AM
simoll marked 13 inline comments as done.
  • Addressed comments.
  • Made ext_vector_type bool un-castable (as any other ext_vector_type) - with tests.
  • Made bool vector element access illegal - with tests.
  • Rebased.
simoll added inline comments.Tue, Nov 3, 6:16 AM
clang/docs/LanguageExtensions.rst
488–489

Is the rule "The size and alignment are both the number of bits rounded up to the next power of two, but the alignment is at most the maximum vector alignment of the target." ?

Yes and i like your description better.

clang/lib/AST/TypePrinter.cpp
654 ↗(On Diff #296470)

Undid this change. There is a separate function for printExtVectorBefore that works fine as is.

clang/lib/CodeGen/CGExpr.cpp
2080–2081

IRStoreTy is used here and below

clang/lib/CodeGen/CGExprScalar.cpp
2039–2041

The ext_vector_type does not support casting and we do not need it for the bool vector type - i've removed this code path.

Any comments for this one? Is this good to go?