[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
https://github.com/frasercrmck updated https://github.com/llvm/llvm-project/pull/119423 >From 30b30fd3c74e41363984ae1055470b9e37d3ee20 Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Tue, 10 Dec 2024 17:41:07 + Subject: [PATCH 1/3] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat These builtins would unconditionally perform the usual arithmetic conversions on promotable scalar integer arguments. This meant in practice that char and short arguments were promoted to int, and the operation was truncated back down afterwards. This in effect silently replaced a saturating add/sub with a regular add/sub, which is not intuitive (or intended) behaviour. With this patch, promotable scalar integer types are not promoted to int, but are kept intact. If the types differ, the smaller integer is promoted to the larger one. The signedness of the operation matches the larger integer type. No change is made to vector types, which are both not promoted and whose element types must match. --- clang/lib/Sema/SemaChecking.cpp | 38 +++ .../test/CodeGen/builtins-elementwise-math.c | 98 ++- 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index a248a6b53b0d06c..6107eb854fb95e7 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -2765,6 +2765,44 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID, // types only. case Builtin::BI__builtin_elementwise_add_sat: case Builtin::BI__builtin_elementwise_sub_sat: { +if (checkArgCount(TheCall, 2)) + return ExprError(); +ExprResult LHS = TheCall->getArg(0); +ExprResult RHS = TheCall->getArg(1); +QualType LHSType = LHS.get()->getType().getUnqualifiedType(); +QualType RHSType = RHS.get()->getType().getUnqualifiedType(); +// If both LHS/RHS are promotable integer types, do not perform the usual +// conversions - we must keep the saturating operation at the correct +// bitwidth. +if (Context.isPromotableIntegerType(LHSType) && +Context.isPromotableIntegerType(RHSType)) { + // First, convert each argument to an r-value. + ExprResult ResLHS = DefaultFunctionArrayLvalueConversion(LHS.get()); + if (ResLHS.isInvalid()) +return ExprError(); + LHS = ResLHS.get(); + + ExprResult ResRHS = DefaultFunctionArrayLvalueConversion(RHS.get()); + if (ResRHS.isInvalid()) +return ExprError(); + RHS = ResRHS.get(); + + LHSType = LHS.get()->getType().getUnqualifiedType(); + RHSType = RHS.get()->getType().getUnqualifiedType(); + + // If the two integer types are not of equal order, cast the smaller + // integer one to the larger one + if (int Order = Context.getIntegerTypeOrder(LHSType, RHSType); Order == 1) +RHS = ImpCastExprToType(RHS.get(), LHSType, CK_IntegralCast); + else if (Order == -1) +LHS = ImpCastExprToType(LHS.get(), RHSType, CK_IntegralCast); + + TheCall->setArg(0, LHS.get()); + TheCall->setArg(1, RHS.get()); + TheCall->setType(LHS.get()->getType().getUnqualifiedType()); + break; +} + if (BuiltinElementwiseMath(TheCall)) return ExprError(); diff --git a/clang/test/CodeGen/builtins-elementwise-math.c b/clang/test/CodeGen/builtins-elementwise-math.c index 7f6b5f26eb93070..4ac6fe18c4d5a36 100644 --- a/clang/test/CodeGen/builtins-elementwise-math.c +++ b/clang/test/CodeGen/builtins-elementwise-math.c @@ -68,7 +68,10 @@ void test_builtin_elementwise_add_sat(float f1, float f2, double d1, double d2, long long int i2, si8 vi1, si8 vi2, unsigned u1, unsigned u2, u4 vu1, u4 vu2, _BitInt(31) bi1, _BitInt(31) bi2, - unsigned _BitInt(55) bu1, unsigned _BitInt(55) bu2) { + unsigned _BitInt(55) bu1, unsigned _BitInt(55) bu2, + char c1, char c2, unsigned char uc1, + unsigned char uc2, short s1, short s2, + unsigned short us1, unsigned short us2) { // CHECK: [[I1:%.+]] = load i64, ptr %i1.addr, align 8 // CHECK-NEXT: [[I2:%.+]] = load i64, ptr %i2.addr, align 8 // CHECK-NEXT: call i64 @llvm.sadd.sat.i64(i64 [[I1]], i64 [[I2]]) @@ -114,6 +117,50 @@ void test_builtin_elementwise_add_sat(float f1, float f2, double d1, double d2, // CHECK: store i64 98, ptr %i1.addr, align 8 i1 = __builtin_elementwise_add_sat(1, 'a'); + + // CHECK: [[C1:%.+]] = load i8, ptr %c1.addr, align 1 + // CHECK-NEXT: [[C2:%.+]] = load i8, ptr %c2.addr, align 1 + // CHECK-NEXT: call i8 @llvm.sadd.sat.i8(i8 [[C1]], i8 [[C2]]) + c1 = __builtin_elementwise_add_sat(c1, c2); + + // CHECK: [[UC1:%.+]] = load i8, ptr %uc1.addr, a
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
frasercrmck wrote: RFC: https://discourse.llvm.org/t/rfc-change-behaviour-of-elementwise-builtins-on-scalar-integer-types/83725. I don't expect anyone to say anything at all, tbh, especially during the holidays. > If people aren't using the edge cases, they won't notice if we start > error'ing out. If they are, it's pretty easy to adapt the code to build with > both old and new compilers. A quick GitHub search shows exactly one user of > the builtins outside of clang itself, and that code uses a vector input. That's useful context, thank you > We should probably write a release note for this in any case. Makes sense. I'll implement the RFC in the new year. We might run into further questions about what to do with enums and other things currently handled for us by the usual unary conversions. I think we'll have to roll our own solution as I haven't seen anything suitable. My knowledge of Sema isn't great, though. https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
efriedma-quic wrote: If people aren't using the edge cases, they won't notice if we start error'ing out. If they are, it's pretty easy to adapt the code to build with both old and new compilers. A quick GitHub search shows exactly one user of the builtins outside of clang itself, and that code uses a vector input. Maybe makes sense to RFC it for wider exposure, but I doubt anyone will object. We should probably write a release note for this in any case. https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
frasercrmck wrote: > Even with this fix, the behavior with mixed types still seems really > confusing, especially if you mix signed/unsigned inputs. Can we address that > somehow? I totally agree. The (elementwise) builtins won't let you mix `ext_vector_type`s of different signs or element sizes, so maybe we should extend logic that to all scalars too? Ban mixed signs? Ban mixed sizes entirely? This would be a larger change that would probably need documenting - an RFC? Worth noting perhaps that maybe the current vector behaviour depends on the type of vectors you use (OpenCL, AltiVec, GCC, NEON, SVE) - I haven't checked that. I note also that the documentation says that `For scalar types, consider the operation applied to a vector with a single element.`. This is misleading, as the usual implicit conversions don't behave the same way for scalars as they do for vectors. CC @arsenm - might be of interest to you. https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
efriedma-quic wrote: Even with this fix, the behavior with mixed types still seems really confusing, especially if you mix signed/unsigned inputs. Can we address that somehow? https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
frasercrmck wrote: > I thought bitreverse/popcount are only supposed to work with unsigned types? Good question. The [documentation](https://clang.llvm.org/docs/LanguageExtensions.html) doesn't mention they're only for signed types, as `abs` does, for example. So if there is such a restriction, it's not documented, nor checked in the code. Tangentially, in OpenCL (where I'm coming from) there's no restriction on `popcount` to be only on unsigned types. https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
RKSimon wrote: I thought bitreverse/popcount are only supposed to work with unsigned types? https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
frasercrmck wrote: I just realised this is also a problem with the `bitreverse` and `popcount` builtins on signed types for the same reason. https://github.com/llvm/llvm-project/pull/119423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
llvmbot wrote: @llvm/pr-subscribers-clang Author: Fraser Cormack (frasercrmck) Changes These builtins would unconditionally perform the usual arithmetic conversions on promotable scalar integer arguments. This meant in practice that char and short arguments were promoted to int, and the operation was truncated back down afterwards. This in effect silently replaced a saturating add/sub with a regular add/sub, which is not intuitive (or intended) behaviour. With this patch, promotable scalar integer types are not promoted to int, but are kept intact. If the types differ, the smaller integer is promoted to the larger one. The signedness of the operation matches the larger integer type. No change is made to vector types, which are both not promoted and whose element types must match. --- Full diff: https://github.com/llvm/llvm-project/pull/119423.diff 2 Files Affected: - (modified) clang/lib/Sema/SemaChecking.cpp (+38) - (modified) clang/test/CodeGen/builtins-elementwise-math.c (+96-2) ``diff diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index a248a6b53b0d06..6107eb854fb95e 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -2765,6 +2765,44 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID, // types only. case Builtin::BI__builtin_elementwise_add_sat: case Builtin::BI__builtin_elementwise_sub_sat: { +if (checkArgCount(TheCall, 2)) + return ExprError(); +ExprResult LHS = TheCall->getArg(0); +ExprResult RHS = TheCall->getArg(1); +QualType LHSType = LHS.get()->getType().getUnqualifiedType(); +QualType RHSType = RHS.get()->getType().getUnqualifiedType(); +// If both LHS/RHS are promotable integer types, do not perform the usual +// conversions - we must keep the saturating operation at the correct +// bitwidth. +if (Context.isPromotableIntegerType(LHSType) && +Context.isPromotableIntegerType(RHSType)) { + // First, convert each argument to an r-value. + ExprResult ResLHS = DefaultFunctionArrayLvalueConversion(LHS.get()); + if (ResLHS.isInvalid()) +return ExprError(); + LHS = ResLHS.get(); + + ExprResult ResRHS = DefaultFunctionArrayLvalueConversion(RHS.get()); + if (ResRHS.isInvalid()) +return ExprError(); + RHS = ResRHS.get(); + + LHSType = LHS.get()->getType().getUnqualifiedType(); + RHSType = RHS.get()->getType().getUnqualifiedType(); + + // If the two integer types are not of equal order, cast the smaller + // integer one to the larger one + if (int Order = Context.getIntegerTypeOrder(LHSType, RHSType); Order == 1) +RHS = ImpCastExprToType(RHS.get(), LHSType, CK_IntegralCast); + else if (Order == -1) +LHS = ImpCastExprToType(LHS.get(), RHSType, CK_IntegralCast); + + TheCall->setArg(0, LHS.get()); + TheCall->setArg(1, RHS.get()); + TheCall->setType(LHS.get()->getType().getUnqualifiedType()); + break; +} + if (BuiltinElementwiseMath(TheCall)) return ExprError(); diff --git a/clang/test/CodeGen/builtins-elementwise-math.c b/clang/test/CodeGen/builtins-elementwise-math.c index 7f6b5f26eb9307..4ac6fe18c4d5a3 100644 --- a/clang/test/CodeGen/builtins-elementwise-math.c +++ b/clang/test/CodeGen/builtins-elementwise-math.c @@ -68,7 +68,10 @@ void test_builtin_elementwise_add_sat(float f1, float f2, double d1, double d2, long long int i2, si8 vi1, si8 vi2, unsigned u1, unsigned u2, u4 vu1, u4 vu2, _BitInt(31) bi1, _BitInt(31) bi2, - unsigned _BitInt(55) bu1, unsigned _BitInt(55) bu2) { + unsigned _BitInt(55) bu1, unsigned _BitInt(55) bu2, + char c1, char c2, unsigned char uc1, + unsigned char uc2, short s1, short s2, + unsigned short us1, unsigned short us2) { // CHECK: [[I1:%.+]] = load i64, ptr %i1.addr, align 8 // CHECK-NEXT: [[I2:%.+]] = load i64, ptr %i2.addr, align 8 // CHECK-NEXT: call i64 @llvm.sadd.sat.i64(i64 [[I1]], i64 [[I2]]) @@ -114,6 +117,50 @@ void test_builtin_elementwise_add_sat(float f1, float f2, double d1, double d2, // CHECK: store i64 98, ptr %i1.addr, align 8 i1 = __builtin_elementwise_add_sat(1, 'a'); + + // CHECK: [[C1:%.+]] = load i8, ptr %c1.addr, align 1 + // CHECK-NEXT: [[C2:%.+]] = load i8, ptr %c2.addr, align 1 + // CHECK-NEXT: call i8 @llvm.sadd.sat.i8(i8 [[C1]], i8 [[C2]]) + c1 = __builtin_elementwise_add_sat(c1, c2); + + // CHECK: [[UC1:%.+]] = load i8, ptr %uc1.addr, align 1 + // CHECK-NEXT: [[UC2:%.+]] = load i8, ptr %uc2.addr, align 1 + // CHECK-NEXT: call i8 @llvm.uadd.sat.i8(i8 [[UC1]], i8 [[UC2]]) + uc1 = __builtin_elementwi
[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)
https://github.com/frasercrmck created https://github.com/llvm/llvm-project/pull/119423 These builtins would unconditionally perform the usual arithmetic conversions on promotable scalar integer arguments. This meant in practice that char and short arguments were promoted to int, and the operation was truncated back down afterwards. This in effect silently replaced a saturating add/sub with a regular add/sub, which is not intuitive (or intended) behaviour. With this patch, promotable scalar integer types are not promoted to int, but are kept intact. If the types differ, the smaller integer is promoted to the larger one. The signedness of the operation matches the larger integer type. No change is made to vector types, which are both not promoted and whose element types must match. >From 98d796a81960b231ab02fd7aba78e33e5a176fee Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Tue, 10 Dec 2024 17:41:07 + Subject: [PATCH] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat These builtins would unconditionally perform the usual arithmetic conversions on promotable scalar integer arguments. This meant in practice that char and short arguments were promoted to int, and the operation was truncated back down afterwards. This in effect silently replaced a saturating add/sub with a regular add/sub, which is not intuitive (or intended) behaviour. With this patch, promotable scalar integer types are not promoted to int, but are kept intact. If the types differ, the smaller integer is promoted to the larger one. The signedness of the operation matches the larger integer type. No change is made to vector types, which are both not promoted and whose element types must match. --- clang/lib/Sema/SemaChecking.cpp | 38 +++ .../test/CodeGen/builtins-elementwise-math.c | 98 ++- 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index a248a6b53b0d06..6107eb854fb95e 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -2765,6 +2765,44 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID, // types only. case Builtin::BI__builtin_elementwise_add_sat: case Builtin::BI__builtin_elementwise_sub_sat: { +if (checkArgCount(TheCall, 2)) + return ExprError(); +ExprResult LHS = TheCall->getArg(0); +ExprResult RHS = TheCall->getArg(1); +QualType LHSType = LHS.get()->getType().getUnqualifiedType(); +QualType RHSType = RHS.get()->getType().getUnqualifiedType(); +// If both LHS/RHS are promotable integer types, do not perform the usual +// conversions - we must keep the saturating operation at the correct +// bitwidth. +if (Context.isPromotableIntegerType(LHSType) && +Context.isPromotableIntegerType(RHSType)) { + // First, convert each argument to an r-value. + ExprResult ResLHS = DefaultFunctionArrayLvalueConversion(LHS.get()); + if (ResLHS.isInvalid()) +return ExprError(); + LHS = ResLHS.get(); + + ExprResult ResRHS = DefaultFunctionArrayLvalueConversion(RHS.get()); + if (ResRHS.isInvalid()) +return ExprError(); + RHS = ResRHS.get(); + + LHSType = LHS.get()->getType().getUnqualifiedType(); + RHSType = RHS.get()->getType().getUnqualifiedType(); + + // If the two integer types are not of equal order, cast the smaller + // integer one to the larger one + if (int Order = Context.getIntegerTypeOrder(LHSType, RHSType); Order == 1) +RHS = ImpCastExprToType(RHS.get(), LHSType, CK_IntegralCast); + else if (Order == -1) +LHS = ImpCastExprToType(LHS.get(), RHSType, CK_IntegralCast); + + TheCall->setArg(0, LHS.get()); + TheCall->setArg(1, RHS.get()); + TheCall->setType(LHS.get()->getType().getUnqualifiedType()); + break; +} + if (BuiltinElementwiseMath(TheCall)) return ExprError(); diff --git a/clang/test/CodeGen/builtins-elementwise-math.c b/clang/test/CodeGen/builtins-elementwise-math.c index 7f6b5f26eb9307..4ac6fe18c4d5a3 100644 --- a/clang/test/CodeGen/builtins-elementwise-math.c +++ b/clang/test/CodeGen/builtins-elementwise-math.c @@ -68,7 +68,10 @@ void test_builtin_elementwise_add_sat(float f1, float f2, double d1, double d2, long long int i2, si8 vi1, si8 vi2, unsigned u1, unsigned u2, u4 vu1, u4 vu2, _BitInt(31) bi1, _BitInt(31) bi2, - unsigned _BitInt(55) bu1, unsigned _BitInt(55) bu2) { + unsigned _BitInt(55) bu1, unsigned _BitInt(55) bu2, + char c1, char c2, unsigned char uc1, + unsigned char uc2, short s1, short s2, + unsigned short us1, unsigned sh