Page MenuHomePhabricator

[Clang] Allow "ext_vector_type" applied to Booleans
ClosedPublic

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

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
478

Comment talks about bool8 but we defined the type bool4.

500–501

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
1986

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
1706–1742
1785–1790

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

1848–1849
clang/lib/CodeGen/CGExprScalar.cpp
2159

"Otw"?

2163–2165

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
7527–7529

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

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

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

clang/lib/CodeGen/CGExpr.cpp
2096–2097

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

clang/lib/CodeGen/CodeGenTypes.cpp
103

dyn_cast -> cast

simoll updated this revision to Diff 302559.Nov 3 2020, 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.Nov 3 2020, 6:16 AM
clang/docs/LanguageExtensions.rst
500–501

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
2096–2097

IRStoreTy is used here and below

clang/lib/CodeGen/CGExprScalar.cpp
2163–2165

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?

AFAIK all comments were addressed. Could you have another look at/LGTM this?

kaz7 added a comment.Dec 26 2020, 5:11 AM

We are using this patch in llvm/clang for VE and the patch is really helpful to implement intrinsic instructions using vector mask registers. This works fine in backend since backend supports vXi1. I wish people working on clang think this patch is reasonable.

clang/docs/LanguageExtensions.rst
478

This is not fixed yet, sizeof(bool8).

simoll updated this revision to Diff 314568.Jan 5 2021, 4:44 AM
simoll marked an inline comment as done.

NFC

  • Use bool4 in docs.
  • Rebased,
Matt added a subscriber: Matt.May 26 2021, 12:12 PM
simoll updated this revision to Diff 398115.Jan 7 2022, 5:50 AM

Rebased onto vector conditionals changes

kaz7 added a comment.EditedMar 6 2022, 5:20 AM

At the beginning, this implementation extends vector_type attribute which is GCC's attribute. So, this may cause future conflicts with GCC when they extend it. But, now this patch uses it's own ext_vector_type attribute. So, basically this modification is safe against to the C/C++ future extension and the GCC future extension, in my honest opinion.

Is it OK to accept this patch? Or is there anything we need to consider? I understand that this is a language extension, so it not easy to say OK... But, this patch spent 1 year and a half almost.

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2022, 5:20 AM

At the beginning, this implementation extends vector_type attribute which is GCC's attribute. So, this may cause future conflicts with GCC when they extend it. But, now this patch uses it's own ext_vector_type attribute. So, basically this modification is safe against to the C/C++ future extension and the GCC future extension, in my honest opinion.

Is it OK to accept this patch? Or is there anything we need to consider? I understand that this is a language extension, so it not easy to say OK... But, this patch spent 1 year and a half almost.

At a minimum, I think the patch needs to be rebased onto the current trunk. However, I don't know of a reason why this patch cannot proceed.

I'd be curious whether the codegen for ext_vector_type of N bools is the same as for N unsigned _BitInt(1) (naively, I'd expect them to be equivalent).

At the beginning, this implementation extends vector_type attribute which is GCC's attribute. So, this may cause future conflicts with GCC when they extend it. But, now this patch uses it's own ext_vector_type attribute. So, basically this modification is safe against to the C/C++ future extension and the GCC future extension, in my honest opinion.

Is it OK to accept this patch? Or is there anything we need to consider? I understand that this is a language extension, so it not easy to say OK... But, this patch spent 1 year and a half almost.

At a minimum, I think the patch needs to be rebased onto the current trunk. However, I don't know of a reason why this patch cannot proceed.

It's great to get feedback on this patch! I am rebasing right now.

I'd be curious whether the codegen for ext_vector_type of N bools is the same as for N unsigned _BitInt(1) (naively, I'd expect them to be equivalent).

bool vectors are for <n x i1>-typed masks in predicated SIMD code - _BitInt is for iN integer arithmetic.
For either type, the memory representation is packed bits with possible differences in alignment, padding.
So, the difference is in the intended usage and what kind of execution units/operands we expect to live the data near by.

At the beginning, this implementation extends vector_type attribute which is GCC's attribute. So, this may cause future conflicts with GCC when they extend it. But, now this patch uses it's own ext_vector_type attribute. So, basically this modification is safe against to the C/C++ future extension and the GCC future extension, in my honest opinion.

Is it OK to accept this patch? Or is there anything we need to consider? I understand that this is a language extension, so it not easy to say OK... But, this patch spent 1 year and a half almost.

At a minimum, I think the patch needs to be rebased onto the current trunk. However, I don't know of a reason why this patch cannot proceed.

It's great to get feedback on this patch! I am rebasing right now.

I'd be curious whether the codegen for ext_vector_type of N bools is the same as for N unsigned _BitInt(1) (naively, I'd expect them to be equivalent).

bool vectors are for <n x i1>-typed masks in predicated SIMD code - _BitInt is for iN integer arithmetic.
For either type, the memory representation is packed bits with possible differences in alignment, padding.
So, the difference is in the intended usage and what kind of execution units/operands we expect to live the data near by.

Okay, that sounds like what I'd hoped for, thank you!

Sema bits look pretty reasonable to me (I don't feel qualified to review the codegen bits).

clang/lib/Sema/SemaExpr.cpp
10151–10152

No need for the parens, but should that && have been ||?

clang/lib/Sema/SemaExprCXX.cpp
6053

bool is already covered by isIntegralType().

simoll updated this revision to Diff 415153.Mar 14 2022, 10:34 AM
simoll marked an inline comment as done.

Simplify following comments. Thx :)

clang/lib/Sema/SemaExpr.cpp
10151–10152

The && is intentional. We cannot step into this if AllowBoolOperation is true.

aaron.ballman added inline comments.Mar 14 2022, 11:14 AM
clang/lib/Sema/SemaExpr.cpp
10151–10152

Yes, but don't we want to step into it if *either* operand is a vector boolean type when bool operations are not allowed? (I'm confused by the !AllowBoolOperations but then allowing them anyway if only one operand is a vector bool.)

simoll updated this revision to Diff 415392.Mar 15 2022, 5:39 AM

Blocking of arithmetic on bool vectors extends to the case where only one operand is a bool vector (and the other is, eg, a vector of int). This was still caught before during sema because the integer vector operand would have been implicitly truncated down to bool, which always has been illegal.
This change results in a different error message for the mixed bool-and-non-bool arithmetic case and a more explicit check in the code ("|| instead of &&").

simoll marked an inline comment as done.Mar 15 2022, 5:39 AM
simoll updated this revision to Diff 415400.Mar 15 2022, 6:01 AM

Make the sema check for bool vector arithmetic specific to ext_vector_type bool. This assures that we do not interfere with scalar bool + int vector arithmetic sema (vector.cpp test was failing before).

Thanks! Sema bits now LGTM

Thanks! Sema bits now LGTM

Awesome! Who would be a good fit for the codegen parts of this patch?

Thanks! Sema bits now LGTM

Awesome! Who would be a good fit for the codegen parts of this patch?

Ping @craig.topper and @erichkeane as they both know CodeGen better than I do, or can add other reviewers to cover those bits.

Everything in the CodeGen looks fine, but I didn't see anything for constant evaluation? Can you ensure that the ExprConstant evaluation for Vectors still works for this type (or, exclude it from participating in constant expressions entirely)?

simoll updated this revision to Diff 415509.Mar 15 2022, 11:08 AM

Check IR generated for constexpr of bool vectors. This actually revealed a bug in the bool vector sema (bool vector comparisons would result in byte sized elements). Thx!

simoll added inline comments.Mar 15 2022, 11:09 AM
clang/test/SemaCXX/constexpr-vectors.cpp
680

@erichkeane This is the constexpr test you were looking for

erichkeane accepted this revision.Mar 15 2022, 11:12 AM
This revision is now accepted and ready to land.Mar 15 2022, 11:12 AM
This revision was landed with ongoing or failed builds.Mar 16 2022, 3:12 AM
This revision was automatically updated to reflect the committed changes.