[Clang] Add __builtin_elementwise_ceil

[junaire]

Summary

This patch implements one of the missing builtin functions specified in https://reviews.llvm.org/D111529

[fhahn]

Thanks for the patch! I left a suggestion inline.

clang/lib/Sema/SemaChecking.cpp

16734

If I understand correctly, this is similar to SemaBuiltinElementwiseMathOneArg, but with the additional restriction to only allow floating point element types?

Do you think it would be possible to extend SemaBuiltinElementwiseMathOneArg to handle this case directly, without needing to duplicate most of the other logic?

[junaire]

Reuse existing code.

[junaire]

Adjust code style

[fhahn]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11337

There's no need to add a new error message here. err_builtin_invalid_arg_type already uses %select to support different types in the message. You can just add a new option to the %select{} (done by |new option). You need to adjust the index passed to Diag() accordingly.

clang/lib/Sema/SemaChecking.cpp

16734

The latest version s till duplicates most of the logic unfortunately. I think it would be good to allow the caller of SemaBuiltinElementwiseMathOneArg to specify additional restrictions on the argument type. This could be done by adding an extra callback argument, like below:

-bool Sema::SemaBuiltinElementwiseMathOneArg(CallExpr *TheCall) {
+bool Sema::SemaBuiltinElementwiseMathOneArg(
+    CallExpr *TheCall,
+    std::function<bool(QualType ArgTy, SourceLocation ArgLoc)>
+        ExtraCheckArgTy) {
   if (checkArgCount(*this, TheCall, 1))
     return true;

@@ -16707,16 +16734,10 @@ bool Sema::SemaBuiltinElementwiseMathOneArg(CallExpr *TheCall) {

   TheCall->setArg(0, A.get());
   QualType TyA = A.get()->getType();
-  if (checkMathBuiltinElementType(*this, ArgLoc, TyA))
+  if (checkMathBuiltinElementType(*this, ArgLoc, TyA) ||
+      ExtraCheckArgTy(TyA, ArgLoc))
     return true;

-  QualType EltTy = TyA;
-  if (auto *VecTy = EltTy->getAs<VectorType>())
-    EltTy = VecTy->getElementType();
-  if (EltTy->isUnsignedIntegerType())
-    return Diag(ArgLoc, diag::err_builtin_invalid_arg_type)
-           << 1 << /*signed integer or float ty*/ 3 << TyA;
-
   TheCall->setType(TyA);
   return false;
 }

At the call site, it would look something like

-    if (SemaBuiltinElementwiseMathOneArg(TheCall))
+    if (SemaBuiltinElementwiseMathOneArg(
+            TheCall, [this](QualType ArgTy, SourceLocation ArgLoc) -> bool {
+              QualType EltTy = ArgTy;
+              if (auto *VecTy = EltTy->getAs<VectorType>())
+                EltTy = VecTy->getElementType();
+              if (EltTy->isUnsignedIntegerType())
+                return Diag(ArgLoc, diag::err_builtin_invalid_arg_type)
+                       << 1 << /*signed integer or float ty*/ 3 << ArgTy;
+              return false;
+            }))
       return ExprError();
     break;

[junaire]

Apply some awesome suggestions.

[fhahn]

clang/lib/Sema/SemaChecking.cpp

2104

Could you add a comment at the top of the function, saying that this restricts the element type to signed integers or floating point types?

2108

Could you add a comment here saying that this restricts the element type to floating point types only?

16708

Could you swap the TyA and ArgLoc arguments to the function, so it is consistent with the earlier call to checkMathBuiltinElementType? (Something I missed when I posted the suggestion)

[aaron.ballman]

clang/include/clang/Basic/Builtins.def

649

If we're doing ceil, should we be doing floor at the same time given how much overlap there should be in the implementations?

clang/lib/Sema/SemaChecking.cpp

16707–16708

In some ways, it's a not really ideal that we split the type checking like this. SemaBuiltinElementwiseMathOneArg() checks some type validity and the caller checks some more type validity, so there's no one location to know what types are actually expected for any given builtin calling this.

Would it be reasonable to instead hoist all the type checking code out of SemaBuiltinElementwiseMathOneArg() and not pass in a functor to it, but instead check the CallExpr type after the call returns? We could also fix up the function name to be a bit less nonsensical at the same time, like renaming it to PrepareBuiltinElementwiseMathOneArgCall() or something along those lines?

[fhahn]

clang/lib/Sema/SemaChecking.cpp

16707–16708

Would it be reasonable to instead hoist all the type checking code out of SemaBuiltinElementwiseMathOneArg() and not pass in a functor to it, but instead check the CallExpr type after the call returns?

Yeah that sounds better!

[junaire]

I didn't know if I understand Aaron's words right, but I think I can update the patch first...

[aaron.ballman]

In D114688#3166870, @junaire wrote:

I didn't know if I understand Aaron's words right, but I think I can update the patch first...

I think you understood my suggestion pretty well, thanks!

clang/lib/Sema/SemaChecking.cpp

2108

I don't think using a lambda here (or below) adds a whole lot of value given that it's only called once. I think it'd cleaner to hoist the lambda logic into the case block directly. WDYT?

[junaire]

No more lambdas

[fhahn]

Thanks for the update, it looks like it should be in line with @aaron.ballman's suggestions!

I think it might be good to split off the refactoring of SemaBuiltinElementwiseMathOneArg-> PrepareBuiltinElementwiseMathOneArgCall and the update for BI__builtin_elementwise_abs into a separate, non functional change (NFC).

clang/lib/Sema/SemaChecking.cpp

2102

nit: typo estricts -> restricts?

2119

nit: typo estricts -> restricts? It should also say _ceil instead of _abs, right?

16728

w

[junaire]

Fix typos

[aaron.ballman]

In D114688#3176433, @fhahn wrote:

I think it might be good to split off the refactoring of SemaBuiltinElementwiseMathOneArg-> PrepareBuiltinElementwiseMathOneArgCall and the update for BI__builtin_elementwise_abs into a separate, non functional change (NFC).

FWIW, I would not be opposed to that, but I don't insist either. LGTM!

[junaire]

In D114688#3176567, @aaron.ballman wrote:

In D114688#3176433, @fhahn wrote:

I think it might be good to split off the refactoring of SemaBuiltinElementwiseMathOneArg-> PrepareBuiltinElementwiseMathOneArgCall and the update for BI__builtin_elementwise_abs into a separate, non functional change (NFC).

FWIW, I would not be opposed to that, but I don't insist either. LGTM!

Hi, @fhahn @aaron.ballman . Thanks for your patient reviews and valuable suggestions! I think I might not apply the extra suggestion as I don't really understand that, sorry... BTW, if you think this patch is ready, could you please commit this for me?

You can use:
Jun Zhang [e-mail]

Cheers

clang/lib/Sema/SemaChecking.cpp

16707–16708

In some ways, it's not really ideal that we split the type checking like this. SemaBuiltinElementwiseMathOneArg() checks some type validity and the caller checks some more type validity, so there's no one location to know what types are actually expected for any given builtin calling this.

Would it be reasonable to instead hoist all the type checking code out of SemaBuiltinElementwiseMathOneArg() and not pass in a functor to it, but instead check the CallExpr type after the call returns? We could also fix up the function name to be a bit less nonsensical at the same time, like renaming it to PrepareBuiltinElementwiseMathOneArgCall() or something along those lines?

Hi, Aaron! Thanks for your review! Unfortunately, I don't get what you mean. :( Sorry about that, I'm completely a newbie...

check the CallExpr type after the call returns?

What does this mean? From my understanding, we only need to check the argument type, right? Or do you mean something like:

  TheCall->setType(TyA);  // inside checking function
...

TheCall->getType(); // call side

Do I understand you correctly? Please don't hesitate to point out any mistake I made, thanks!

16734

If I understand correctly, this is similar to SemaBuiltinElementwiseMathOneArg, but with the additional restriction to only allow floating point element types?

Do you think it would be possible to extend SemaBuiltinElementwiseMathOneArg to handle this case directly, without needing to duplicate most of the other logic?

Hi, Florian, Thanks for your suggestion! I have updated the patch, PTAL :-)

[aaron.ballman]

Thank you for the new functionality, I've committed on your behalf in 8680f951c21e675a110e79c9b8dc59bb94290b01

clang/lib/Sema/SemaChecking.cpp

16707–16708

Hi, Aaron! Thanks for your review! Unfortunately, I don't get what you mean. :( Sorry about that, I'm completely a newbie...

No apologies necessary, we've all started there! :-)

What does this mean? From my understanding, we only need to check the argument type, right? Or do you mean something like:

 TheCall->setType(TyA);  // inside checking function
...

TheCall->getType(); // call side

Do I understand you correctly? Please don't hesitate to point out any mistake I made, thanks!

The way you implemented it and the way I suggested it are functionally the same outcome.

PrepareBuiltinElementwiseMathOneArgCall() eventually calls TheCall->setType(TyA); to set the return type of the call expression to be the same as the argument type. After calling that method, you can either do QualType ArgTy = TheCall->getArg(0)->getType(); as you were doing or do QualType ArgTy = TheCall->getType(); to get that adjusted type.

The way you've written it currently is correct and likely a more readable solution. My suggestion would have ever-so-slightly better performance because it doesn't call getArg(0), but I think we should prefer your approach because it's more readable. That's why I accepted the patch as-is.

Does that clarify?

clang/include/clang/Basic/Builtins.def

 BUILTIN(__builtin_convertvector, "v."   , "nct")
 BUILTIN(__builtin_alloca, "v*z"   , "Fn")
 BUILTIN(__builtin_alloca_with_align, "v*zIz", "Fn")
 BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
 
 BUILTIN(__builtin_elementwise_abs, "v.", "nct")
 BUILTIN(__builtin_elementwise_max, "v.", "nct")
 BUILTIN(__builtin_elementwise_min, "v.", "nct")
+BUILTIN(__builtin_elementwise_ceil, "v.", "nct")
 BUILTIN(__builtin_reduce_max, "v.", "nct")
 BUILTIN(__builtin_reduce_min, "v.", "nct")
 
 BUILTIN(__builtin_matrix_transpose, "v.", "nFt")
 BUILTIN(__builtin_matrix_column_major_load, "v.", "nFt")
 BUILTIN(__builtin_matrix_column_major_store, "v.", "nFt")
 
 // "Overloaded" Atomic operator builtins.  These are overloaded to support data

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_builtin_launder_invalid_arg : Error<
   "'__builtin_launder' is not allowed">;
 
 def err_builtin_invalid_arg_type: Error <
   "%ordinal0 argument must be a "
   "%select{vector, integer or floating point type|matrix|"
    "pointer to a valid matrix element type|"
    "signed integer or floating point type|vector type}1 (was %2)">;
 
+def err_builtin_float_invalid_arg_type: Error <
+  "%ordinal0 argument must be a "
+  "%select{vector or floating point type|matrix|"
+   "pointer to a valid matrix element type|"
+   "floating point type|vector type}1 (was %2)">;
+
 def err_builtin_matrix_disabled: Error<
   "matrix types extension is disabled. Pass -fenable-matrix to enable it">;
 def err_matrix_index_not_integer: Error<
   "matrix %select{row|column}0 index is not an integer">;
 def err_matrix_index_outside_range: Error<
   "matrix %select{row|column}0 index is outside the allowed range [0, %1)">;
 def err_matrix_incomplete_index: Error<
   "single subscript expressions are not allowed for matrix values">;

clang/include/clang/Sema/Sema.h

private:
   bool SemaBuiltinPPCMMACall(CallExpr *TheCall, unsigned BuiltinID,
                              const char *TypeDesc);
 
   bool CheckPPCMMAType(QualType Type, SourceLocation TypeLoc);
 
   bool SemaBuiltinElementwiseMath(CallExpr *TheCall);
   bool SemaBuiltinElementwiseMathOneArg(CallExpr *TheCall);
   bool SemaBuiltinReduceMath(CallExpr *TheCall);
+  bool SemaBuiltinElementwiseMathFloatArg(CallExpr *TheCall);
+  bool SemaBuiltinElementwiseMathNotUnsignedArg(CallExpr *TheCall);
 
   // Matrix builtin handling.
   ExprResult SemaBuiltinMatrixTranspose(CallExpr *TheCall,
                                         ExprResult CallResult);
   ExprResult SemaBuiltinMatrixColumnMajorLoad(CallExpr *TheCall,
                                               ExprResult CallResult);
   ExprResult SemaBuiltinMatrixColumnMajorStore(CallExpr *TheCall,
                                                ExprResult CallResult);

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_elementwise_abs: {
     if (Op0->getType()->isIntOrIntVectorTy())
       Result = Builder.CreateBinaryIntrinsic(
           llvm::Intrinsic::abs, Op0, Builder.getFalse(), nullptr, "elt.abs");
     else
       Result = Builder.CreateUnaryIntrinsic(llvm::Intrinsic::fabs, Op0, nullptr,
                                             "elt.abs");
     return RValue::get(Result);
   }
+
+  case Builtin::BI__builtin_elementwise_ceil: {
+    Value *Op0 = EmitScalarExpr(E->getArg(0));
+    Value *Result = Builder.CreateUnaryIntrinsic(llvm::Intrinsic::ceil, Op0,
+                                                 nullptr, "elt.ceil");
+    return RValue::get(Result);
+  }
+
   case Builtin::BI__builtin_elementwise_max: {
     Value *Op0 = EmitScalarExpr(E->getArg(0));
     Value *Op1 = EmitScalarExpr(E->getArg(1));
     Value *Result;
     if (Op0->getType()->isIntOrIntVectorTy()) {
       QualType Ty = E->getArg(0)->getType();
       if (auto *VecTy = Ty->getAs<VectorType>())
         Ty = VecTy->getElementType();

clang/lib/Sema/SemaChecking.cpp

if (!TheCall->getArg(0)->isValueDependent() &&
           << ((BuiltinID == Builtin::BI__builtin_return_address)
                   ? "__builtin_return_address"
                   : "__builtin_frame_address")
           << TheCall->getSourceRange();
     break;
   }
 
   case Builtin::BI__builtin_elementwise_abs:
-    if (SemaBuiltinElementwiseMathOneArg(TheCall))
+    if (SemaBuiltinElementwiseMathNotUnsignedArg(TheCall))
+      return ExprError();
+    break;
+  case Builtin::BI__builtin_elementwise_ceil:
+    if (SemaBuiltinElementwiseMathFloatArg(TheCall))
       return ExprError();
     break;
+
   case Builtin::BI__builtin_elementwise_min:
   case Builtin::BI__builtin_elementwise_max:
     if (SemaBuiltinElementwiseMath(TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_reduce_max:
   case Builtin::BI__builtin_reduce_min:
     if (SemaBuiltinReduceMath(TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_matrix_transpose:
     return SemaBuiltinMatrixTranspose(TheCall, TheCallResult);
 
   case Builtin::BI__builtin_matrix_column_major_load:
     return SemaBuiltinMatrixColumnMajorLoad(TheCall, TheCallResult);
 
   case Builtin::BI__builtin_matrix_column_major_store:
     return SemaBuiltinMatrixColumnMajorStore(TheCall, TheCallResult);
bool Sema::SemaBuiltinElementwiseMathOneArg(CallExpr *TheCall) {
 
   ExprResult A = UsualUnaryConversions(TheCall->getArg(0));
   SourceLocation ArgLoc = TheCall->getArg(0)->getBeginLoc();
   if (A.isInvalid())
     return true;
 
   TheCall->setArg(0, A.get());
   QualType TyA = A.get()->getType();
   if (checkMathBuiltinElementType(*this, ArgLoc, TyA))
     return true;
 
+  return false;
+}
+
+bool Sema::SemaBuiltinElementwiseMathNotUnsignedArg(CallExpr *TheCall) {
+  if (SemaBuiltinElementwiseMathOneArg(TheCall))
+    return true;
+
+  ExprResult A = UsualUnaryConversions(TheCall->getArg(0));
+  QualType TyA = A.get()->getType();
+
+  SourceLocation ArgLoc = TheCall->getArg(0)->getBeginLoc();
   QualType EltTy = TyA;
-  if (auto *VecTy = EltTy->getAs<VectorType>())
+
+  if (auto *VecTy = EltTy->getAs<VectorType>()) {
     EltTy = VecTy->getElementType();
+  }
   if (EltTy->isUnsignedIntegerType())
     return Diag(ArgLoc, diag::err_builtin_invalid_arg_type)
            << 1 << /*signed integer or float ty*/ 3 << TyA;
 
   TheCall->setType(TyA);
   return false;
 }
 
+bool Sema::SemaBuiltinElementwiseMathFloatArg(CallExpr *TheCall) {
+  if (SemaBuiltinElementwiseMathOneArg(TheCall))
+    return true;
+
+  ExprResult A = UsualUnaryConversions(TheCall->getArg(0));
+  QualType TyA = A.get()->getType();
+
+  SourceLocation ArgLoc = TheCall->getArg(0)->getBeginLoc();
+  QualType EltTy = TyA;
+
+  if (auto *VecTy = EltTy->getAs<VectorType>())
+    EltTy = VecTy->getElementType();
+  if (EltTy->isUnsignedIntegerType() || EltTy->isIntegerType())
+    return Diag(ArgLoc, diag::err_builtin_float_invalid_arg_type)
+           << 1 << /*float ty*/ 3 << TyA;
+
+  TheCall->setType(TyA);
+  return false;
+}
+
 bool Sema::SemaBuiltinElementwiseMath(CallExpr *TheCall) {
   if (checkArgCount(*this, TheCall, 2))
     return true;
 
   ExprResult A = TheCall->getArg(0);
   ExprResult B = TheCall->getArg(1);
   // Do standard promotions between the two arguments, returning their common
   // type.

clang/test/CodeGen/builtins-elementwise-math.c

void test_builtin_elementwise_min(float f1, float f2, double d1, double d2,
   // CHECK-NEXT: call <4 x float> @llvm.minnum.v4f32(<4 x float> [[VF2]], <4 x float> [[CVF1]])
   vf1 = __builtin_elementwise_min(vf2, cvf1);
 
   // CHECK:      [[IAS1:%.+]] = load i32, i32 addrspace(1)* @int_as_one, align 4
   // CHECK-NEXT: [[B:%.+]] = load i32, i32* @b, align 4
   // CHECK-NEXT: call i32 @llvm.smin.i32(i32 [[IAS1]], i32 [[B]])
   int_as_one = __builtin_elementwise_min(int_as_one, b);
 }
+
+void test_builtin_elementwise_ceil(float f1, float f2, double d1, double d2,
+                                   float4 vf1, float4 vf2, si8 vi1, si8 vi2,
+                                   long long int i1, long long int i2, short si) {
+  // CHECK-LABEL: define void @test_builtin_elementwise_ceil(
+  // CHECK:      [[F1:%.+]] = load float, float* %f1.addr, align 4
+  // CHECK-NEXT:  call float @llvm.ceil.f32(float [[F1]])
+  f2 = __builtin_elementwise_ceil(f1);
+
+  // CHECK:      [[D1:%.+]] = load double, double* %d1.addr, align 8
+  // CHECK-NEXT: call double @llvm.ceil.f64(double [[D1]])
+  d2 = __builtin_elementwise_ceil(d1);
+
+  // CHECK:      [[VF1:%.+]] = load <4 x float>, <4 x float>* %vf1.addr, align 16
+  // CHECK-NEXT: call <4 x float> @llvm.ceil.v4f32(<4 x float> [[VF1]])
+  vf2 = __builtin_elementwise_ceil(vf1);
+}

clang/test/Sema/builtins-elementwise-math.c

void test_builtin_elementwise_min(int i, short s, double d, float4 v, int3 iv, int *p) {
   int(ii);
   int j;
   j = __builtin_elementwise_min(i, j);
 
   _Complex float c1, c2;
   c1 = __builtin_elementwise_min(c1, c2);
   // expected-error@-1 {{1st argument must be a vector, integer or floating point type (was '_Complex float')}}
 }
+
+void test_builtin_elementwise_ceil(int i, float f, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
+
+  struct Foo s = __builtin_elementwise_ceil(f);
+  // expected-error@-1 {{initializing 'struct Foo' with an expression of incompatible type 'float'}}
+
+  i = __builtin_elementwise_ceil();
+  // expected-error@-1 {{too few arguments to function call, expected 1, have 0}}
+
+  i = __builtin_elementwise_ceil(i);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
+
+  i = __builtin_elementwise_ceil(f, f);
+  // expected-error@-1 {{too many arguments to function call, expected 1, have 2}}
+
+  u = __builtin_elementwise_ceil(u);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned int')}}
+
+  uv = __builtin_elementwise_ceil(uv);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned4' (vector of 4 'unsigned int' values))}}
+}

clang/test/SemaCXX/builtins-elementwise-math.cpp

 
 void test_builtin_elementwise_min() {
   const int a = 2;
   int b = 1;
   static_assert(!is_const<decltype(__builtin_elementwise_min(a, b))>::value);
   static_assert(!is_const<decltype(__builtin_elementwise_min(b, a))>::value);
   static_assert(!is_const<decltype(__builtin_elementwise_min(a, a))>::value);
 }
+
+void test_builtin_elementwise_ceil() {
+  const float a = 42.0;
+  float b = 42.3;
+  static_assert(!is_const<decltype(__builtin_elementwise_ceil(a))>::value);
+  static_assert(!is_const<decltype(__builtin_elementwise_ceil(b))>::value);
+}