[PATCH] D114688: [Clang] Add __builtin_elementwise_ceil

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11337
 
+def err_builtin_float_invalid_arg_type: Error <
+  "%ordinal0 argument must be a "
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16721
 
+bool Sema::SemaBuiltinElementwiseMathFloatArg(CallExpr *TheCall) {
+  if (checkArgCount(*this, TheCall, 1))
----------------
fhahn wrote:
> 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?
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;
```


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

https://reviews.llvm.org/D114688