Skip to content

[clang] Restrict the use of scalar types in vector builtins #119423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jan 29, 2025
Merged

[clang] Restrict the use of scalar types in vector builtins #119423

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f007d7
[clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat
frasercrmck Dec 10, 2024
4d39f93
fix popcount/bitreverse
frasercrmck Dec 11, 2024
479f384
wip: prevent any type mixing
frasercrmck Dec 18, 2024
ad8ccba
update tests
frasercrmck Jan 6, 2025
80a8399
update docs
frasercrmck Jan 6, 2025
453460f
update docs: newline
frasercrmck Jan 6, 2025
f009991
fp conversions
frasercrmck Jan 7, 2025
9d7e79d
forbid mixed enums
frasercrmck Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/docs/LanguageExtensions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,9 @@ Unless specified otherwise operation(±0) = ±0 and operation(±infinity) = ±in

The integer elementwise intrinsics, including ``__builtin_elementwise_popcount``,
``__builtin_elementwise_bitreverse``, ``__builtin_elementwise_add_sat``,
``__builtin_elementwise_sub_sat`` can be called in a ``constexpr`` context.
``__builtin_elementwise_sub_sat`` can be called in a ``constexpr`` context. No
implicit promotion of integer types takes place. The mixing of integer types of
different sizes and signs is forbidden in binary and ternary builtins.

============================================== ====================================================================== =========================================
Name Operation Supported element types
Expand Down
8 changes: 7 additions & 1 deletion clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2323,7 +2323,8 @@ class Sema final : public SemaBase {
const FunctionProtoType *Proto);

/// \param FPOnly restricts the arguments to floating-point types.
bool BuiltinVectorMath(CallExpr *TheCall, QualType &Res, bool FPOnly = false);
std::optional<QualType> BuiltinVectorMath(CallExpr *TheCall,
bool FPOnly = false);
bool BuiltinVectorToScalarMath(CallExpr *TheCall);

void checkLifetimeCaptureBy(FunctionDecl *FDecl, bool IsMemberFunction,
Expand Down Expand Up @@ -7557,6 +7558,11 @@ class Sema final : public SemaBase {
ExprResult DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT,
FunctionDecl *FDecl);

// Check that the usual arithmetic conversions can be performed on this pair
// of expressions that might be of enumeration type.
void checkEnumArithmeticConversions(Expr *LHS, Expr *RHS, SourceLocation Loc,
Sema::ArithConvKind ACK);

// UsualArithmeticConversions - performs the UsualUnaryConversions on it's
// operands and then handles various conversions that are common to binary
// operators (C99 6.3.1.8). If both operands aren't arithmetic, this
Expand Down
80 changes: 47 additions & 33 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14594,11 +14594,18 @@ void Sema::CheckAddressOfPackedMember(Expr *rhs) {
_2, _3, _4));
}

static ExprResult UsualUnaryConversionsNoPromoteInt(Sema &S, Expr *E) {
// Don't promote integer types
if (QualType Ty = E->getType(); S.getASTContext().isPromotableIntegerType(Ty))
return S.DefaultFunctionArrayLvalueConversion(E);
return S.UsualUnaryConversions(E);
Copy link
Collaborator

@efriedma-quic efriedma-quic Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm following correctly, this specifically skips promoting integers... but it allows promoting floats? I think I'd prefer to factor out the unary-float conversions into a separate function, and call that here, for clarity.

Copy link
Contributor Author

@frasercrmck frasercrmck Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, pretty much. UsualUnaryConversions does DefaultFunctionArrayLvalueConversion first, then floating-point specific logic. However, it also handles isPromotableBitFields.

In your other comment you ask about enums, and perhaps bitfields also falls into that category? Do we really need to support these types in vector maths/arithmetic builtins? There are tests for them, so I preserved the existing logic (though seemingly not for bitfields) but I see the argument for removing that support.

Copy link
Contributor Author

@frasercrmck frasercrmck Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps extend UsualUnaryConversions with a default-true bool flag to control the handling of integers/bitfields? I can see that it might be exposing too many internal details to users of UsualUnaryConversions, so I'm not sure. I don't know the best practices of clang/sema design here.

Copy link
Collaborator

@efriedma-quic efriedma-quic Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "usual unary conversions" is not a term used by any specification, so there isn't any real meaning attached to it. I think I'd prefer not to use a boolean, though, because operation you want here is different in a pretty subtle way.

I missed that this also impacts bitfields.

Whether we support enums here probably doesn't really matter, as long as we're consistent. Bitfields are unfortunately weird, due to the way promotion works; we might want to reject due to the potential ambiguity.

Copy link
Contributor Author

@frasercrmck frasercrmck Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, makes sense. I've added Sema::UsualUnaryFPConversions and am using that where appropriate. I didn't want to fold in the use of DefaultFunctionArrayLvalueConversion into that even though that would have saved a bit of duplicated code.

Regarding bitfields: with the current version of this patch (i.e., without the implicit promotion) we are calling the builtin according to the type you specify in the bitfield:

struct StructWithBitfield {
  int i : 4;
  char c: 4;
  short s : 4;
};

__builtin_elementwise_abs(s.i) calls llvm.abs.i32, s.c calls llvm.abs.i8, s.s llvm.abs.i16, etc. I wonder if this is consistent enough behaviour to permit them?

Copy link
Collaborator

@efriedma-quic efriedma-quic Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little weird that the resulting type can be larger than what integer promotion would produce (without this patch, struct S { long l : 4; }; S s; static_assert(sizeof(__builtin_elementwise_abs(s.l)) == 4);). But maybe that's fine.

Copy link
Contributor Author

@frasercrmck frasercrmck Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong opinions either way. I've added some CodeGen tests for bitfields so at least the current behaviour is tested.

}

bool Sema::PrepareBuiltinElementwiseMathOneArgCall(CallExpr *TheCall) {
if (checkArgCount(TheCall, 1))
return true;

ExprResult A = UsualUnaryConversions(TheCall->getArg(0));
ExprResult A = UsualUnaryConversionsNoPromoteInt(*this, TheCall->getArg(0));
if (A.isInvalid())
return true;

Expand All @@ -14613,57 +14620,63 @@ bool Sema::PrepareBuiltinElementwiseMathOneArgCall(CallExpr *TheCall) {
}

bool Sema::BuiltinElementwiseMath(CallExpr *TheCall, bool FPOnly) {
QualType Res;
if (BuiltinVectorMath(TheCall, Res, FPOnly))
return true;
TheCall->setType(Res);
return false;
if (auto Res = BuiltinVectorMath(TheCall, FPOnly); Res.has_value()) {
TheCall->setType(*Res);
return false;
}
return true;
}

bool Sema::BuiltinVectorToScalarMath(CallExpr *TheCall) {
QualType Res;
if (BuiltinVectorMath(TheCall, Res))
std::optional<QualType> Res = BuiltinVectorMath(TheCall);
if (!Res)
return true;

if (auto *VecTy0 = Res->getAs<VectorType>())
if (auto *VecTy0 = (*Res)->getAs<VectorType>())
TheCall->setType(VecTy0->getElementType());
else
TheCall->setType(Res);
TheCall->setType(*Res);

return false;
}

bool Sema::BuiltinVectorMath(CallExpr *TheCall, QualType &Res, bool FPOnly) {
std::optional<QualType> Sema::BuiltinVectorMath(CallExpr *TheCall,
bool FPOnly) {
if (checkArgCount(TheCall, 2))
return true;
return std::nullopt;

ExprResult A = TheCall->getArg(0);
ExprResult B = TheCall->getArg(1);
// Do standard promotions between the two arguments, returning their common
// type.
Res = UsualArithmeticConversions(A, B, TheCall->getExprLoc(), ACK_Comparison);
if (A.isInvalid() || B.isInvalid())
return true;
checkEnumArithmeticConversions(TheCall->getArg(0), TheCall->getArg(1),
TheCall->getExprLoc(), ACK_Comparison);
Copy link
Collaborator

@efriedma-quic efriedma-quic Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changning whether enums are allowed based on the language mode seems unintuitive and unnecessary. Either allow them, or forbid them.

Copy link
Contributor Author

@frasercrmck frasercrmck Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what you mean here, sorry? I assume you mean just in the context of these vector builtins. This call looks like it's just checking the following:

  // C++2a [expr.arith.conv]p1:
  //   If one operand is of enumeration type and the other operand is of a
  //   different enumeration type or a floating-point type, this behavior is
  //   deprecated ([depr.arith.conv.enum]).
  //
  // Warn on this in all language modes. Produce a deprecation warning in C++20.
  // Eventually we will presumably reject these cases (in C++23 onwards?).

So it's not really whether "enums are allowed", but whether you're allowed to mix different enums, or enums with floating-point values, in binary builtins.

I agree the inconsistency here is not desirable, especially as we're not doing the same for the ternary elementwise builtins.

Should we just forbid this in all binary/ternary vector elementwise builtins?

Copy link
Collaborator

@efriedma-quic efriedma-quic Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so.

Copy link
Contributor Author

@frasercrmck frasercrmck Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I've forbidden mixed enum types and have added some tests for them.


QualType TyA = A.get()->getType();
QualType TyB = B.get()->getType();
Expr *Args[2];
for (int I = 0; I < 2; ++I) {
ExprResult Converted =
UsualUnaryConversionsNoPromoteInt(*this, TheCall->getArg(I));
if (Converted.isInvalid())
return std::nullopt;
Args[I] = Converted.get();
}

if (Res.isNull() || TyA.getCanonicalType() != TyB.getCanonicalType())
return Diag(A.get()->getBeginLoc(),
diag::err_typecheck_call_different_arg_types)
<< TyA << TyB;
SourceLocation LocA = Args[0]->getBeginLoc();
QualType TyA = Args[0]->getType();
QualType TyB = Args[1]->getType();

if (TyA.getCanonicalType() != TyB.getCanonicalType()) {
Diag(LocA, diag::err_typecheck_call_different_arg_types) << TyA << TyB;
return std::nullopt;
}

if (FPOnly) {
if (checkFPMathBuiltinElementType(*this, A.get()->getBeginLoc(), TyA, 1))
return true;
if (checkFPMathBuiltinElementType(*this, LocA, TyA, 1))
return std::nullopt;
} else {
if (checkMathBuiltinElementType(*this, A.get()->getBeginLoc(), TyA, 1))
return true;
if (checkMathBuiltinElementType(*this, LocA, TyA, 1))
return std::nullopt;
}

TheCall->setArg(0, A.get());
TheCall->setArg(1, B.get());
return false;
TheCall->setArg(0, Args[0]);
TheCall->setArg(1, Args[1]);
return TyA;
}

bool Sema::BuiltinElementwiseTernaryMath(CallExpr *TheCall,
Expand All @@ -14673,7 +14686,8 @@ bool Sema::BuiltinElementwiseTernaryMath(CallExpr *TheCall,

Expr *Args[3];
for (int I = 0; I < 3; ++I) {
ExprResult Converted = UsualUnaryConversions(TheCall->getArg(I));
ExprResult Converted =
UsualUnaryConversionsNoPromoteInt(*this, TheCall->getArg(I));
if (Converted.isInvalid())
return true;
Args[I] = Converted.get();
Expand Down
41 changes: 20 additions & 21 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,64 +1489,63 @@ static QualType handleFixedPointConversion(Sema &S, QualType LHSTy,

/// Check that the usual arithmetic conversions can be performed on this pair of
/// expressions that might be of enumeration type.
static void checkEnumArithmeticConversions(Sema &S, Expr *LHS, Expr *RHS,
SourceLocation Loc,
Sema::ArithConvKind ACK) {
void Sema::checkEnumArithmeticConversions(Expr *LHS, Expr *RHS,
SourceLocation Loc,
Sema::ArithConvKind ACK) {
// C++2a [expr.arith.conv]p1:
// If one operand is of enumeration type and the other operand is of a
// different enumeration type or a floating-point type, this behavior is
// deprecated ([depr.arith.conv.enum]).
//
// Warn on this in all language modes. Produce a deprecation warning in C++20.
// Eventually we will presumably reject these cases (in C++23 onwards?).
QualType L = LHS->getEnumCoercedType(S.Context),
R = RHS->getEnumCoercedType(S.Context);
QualType L = LHS->getEnumCoercedType(Context),
R = RHS->getEnumCoercedType(Context);
bool LEnum = L->isUnscopedEnumerationType(),
REnum = R->isUnscopedEnumerationType();
bool IsCompAssign = ACK == Sema::ACK_CompAssign;
if ((!IsCompAssign && LEnum && R->isFloatingType()) ||
(REnum && L->isFloatingType())) {
S.Diag(Loc, S.getLangOpts().CPlusPlus26
? diag::err_arith_conv_enum_float_cxx26
: S.getLangOpts().CPlusPlus20
? diag::warn_arith_conv_enum_float_cxx20
: diag::warn_arith_conv_enum_float)
Diag(Loc, getLangOpts().CPlusPlus26 ? diag::err_arith_conv_enum_float_cxx26
: getLangOpts().CPlusPlus20
? diag::warn_arith_conv_enum_float_cxx20
: diag::warn_arith_conv_enum_float)
<< LHS->getSourceRange() << RHS->getSourceRange() << (int)ACK << LEnum
<< L << R;
} else if (!IsCompAssign && LEnum && REnum &&
!S.Context.hasSameUnqualifiedType(L, R)) {
!Context.hasSameUnqualifiedType(L, R)) {
unsigned DiagID;
// In C++ 26, usual arithmetic conversions between 2 different enum types
// are ill-formed.
if (S.getLangOpts().CPlusPlus26)
if (getLangOpts().CPlusPlus26)
DiagID = diag::err_conv_mixed_enum_types_cxx26;
else if (!L->castAs<EnumType>()->getDecl()->hasNameForLinkage() ||
!R->castAs<EnumType>()->getDecl()->hasNameForLinkage()) {
// If either enumeration type is unnamed, it's less likely that the
// user cares about this, but this situation is still deprecated in
// C++2a. Use a different warning group.
DiagID = S.getLangOpts().CPlusPlus20
? diag::warn_arith_conv_mixed_anon_enum_types_cxx20
: diag::warn_arith_conv_mixed_anon_enum_types;
DiagID = getLangOpts().CPlusPlus20
? diag::warn_arith_conv_mixed_anon_enum_types_cxx20
: diag::warn_arith_conv_mixed_anon_enum_types;
} else if (ACK == Sema::ACK_Conditional) {
// Conditional expressions are separated out because they have
// historically had a different warning flag.
DiagID = S.getLangOpts().CPlusPlus20
DiagID = getLangOpts().CPlusPlus20
? diag::warn_conditional_mixed_enum_types_cxx20
: diag::warn_conditional_mixed_enum_types;
} else if (ACK == Sema::ACK_Comparison) {
// Comparison expressions are separated out because they have
// historically had a different warning flag.
DiagID = S.getLangOpts().CPlusPlus20
DiagID = getLangOpts().CPlusPlus20
? diag::warn_comparison_mixed_enum_types_cxx20
: diag::warn_comparison_mixed_enum_types;
} else {
DiagID = S.getLangOpts().CPlusPlus20
DiagID = getLangOpts().CPlusPlus20
? diag::warn_arith_conv_mixed_enum_types_cxx20
: diag::warn_arith_conv_mixed_enum_types;
}
S.Diag(Loc, DiagID) << LHS->getSourceRange() << RHS->getSourceRange()
<< (int)ACK << L << R;
Diag(Loc, DiagID) << LHS->getSourceRange() << RHS->getSourceRange()
<< (int)ACK << L << R;
}
}

Expand All @@ -1557,7 +1556,7 @@ static void checkEnumArithmeticConversions(Sema &S, Expr *LHS, Expr *RHS,
QualType Sema::UsualArithmeticConversions(ExprResult &LHS, ExprResult &RHS,
SourceLocation Loc,
ArithConvKind ACK) {
checkEnumArithmeticConversions(*this, LHS.get(), RHS.get(), Loc, ACK);
checkEnumArithmeticConversions(LHS.get(), RHS.get(), Loc, ACK);

if (ACK != ACK_CompAssign) {
LHS = UsualUnaryConversions(LHS.get());
Expand Down
Loading
Loading