[clang] [clang] Fix sub-integer __builtin_elementwise_(add|sub)_sat (PR #119423)

2024-12-18 Thread Fraser Cormack via cfe-commits

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)

2024-12-18 Thread Fraser Cormack via cfe-commits

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)

2024-12-13 Thread Eli Friedman via cfe-commits

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)

2024-12-12 Thread Fraser Cormack via cfe-commits

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)

2024-12-11 Thread Eli Friedman via cfe-commits

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)

2024-12-11 Thread Fraser Cormack via cfe-commits

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)

2024-12-11 Thread Simon Pilgrim via cfe-commits

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)

2024-12-11 Thread Fraser Cormack via cfe-commits

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)

2024-12-10 Thread via cfe-commits

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)

2024-12-10 Thread Fraser Cormack via cfe-commits

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