[PATCH] D117898: [Clang] Add elementwise saturated add/sub builtins

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:549
+ T __builtin_elementwise_add_sat(T x, T y) return the sum of x and y, clamped to the range of signed or     integer types
+                                           values representable by the bit width of the arguments.
+ T __builtin_elementwise_sub_sat(T x, T y) return the difference of x and y, clamped to the range of        integer types
----------------
craig.topper wrote:
> Not sure if I'm reading this right due to the columns, but is "unsigned" missing after the "signed or"
This reads strangely to me as well. "..., clamped to the range of signed or integer types unsigned values representable by.."


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3157-3184
+  case Builtin::BI__builtin_elementwise_add_sat: {
+    Value *Op0 = EmitScalarExpr(E->getArg(0));
+    Value *Op1 = EmitScalarExpr(E->getArg(1));
+    Value *Result;
+    assert(Op0->getType()->isIntOrIntVectorTy() && "integer type expected");
+    QualType Ty = E->getArg(0)->getType();
+    if (auto *VecTy = Ty->getAs<VectorType>())
----------------
Almost all of this logic is shared (except for picking the intrinsic), should it be combined?


================
Comment at: clang/test/CodeGen/builtins-elementwise-math.c:117
+  // CHECK-NEXT: call <4 x i32> @llvm.usub.sat.v4i32(<4 x i32> [[VU1]], <4 x i32> [[VU2]])
+  vu1 = __builtin_elementwise_sub_sat(vu1, vu2);
+
----------------
fhahn wrote:
> It might be good to have tests where one argument is signed and the other unsigned as well. Those appear to be missing for other builtins as well unfortunately.
I think it'd be good to have some codegen tests for `_BitInt` to make sure the behavior is correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117898/new/

https://reviews.llvm.org/D117898