[PATCH] D140992: clang: Add __builtin_elementwise_fma

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.
Herald added a subscriber: StephenFan.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:2615
     QualType ArgTy = TheCall->getArg(0)->getType();
-    QualType EltTy = ArgTy;
-
-    if (auto *VecTy = EltTy->getAs<VectorType>())
-      EltTy = VecTy->getElementType();
-    if (!EltTy->isFloatingType()) {
-      Diag(TheCall->getArg(0)->getBeginLoc(),
-           diag::err_builtin_invalid_arg_type)
-          << 1 << /* float ty*/ 5 << ArgTy;
-
+    if (checkFPMathBuiltinElementType(*this, TheCall->getArg(0)->getBeginLoc(),
+                                      ArgTy, 1))
----------------
arsenm wrote:
> bob80905 wrote:
> > This change appears to allow more types (such as integers) as args for this set of builtins in the above cases, where before the behavior was to just allow floats.
> > Is this intended behavior?
> > 
> No? test/Sema/builtins-elementwise-math.c already checks all of these reject integers and passes
As best I can tell, it actually allows *less* types. The old code was calling `!EltTy->isFloatingType()` and the new code is calling `!EltTy->isRealFloatingType()`, so the old code would allow a complex float while the new code prohibits it. Is that intentional?

We should add some explicit test coverage for how these builtins work with complex types.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:17761-17766
+  for (int I = 0; I < 3; ++I) {
+    ExprResult Converted = UsualUnaryConversions(TheCall->getArg(I));
+    if (Converted.isInvalid())
+      return true;
+    Args[I] = Converted.get();
+  }
----------------
This will cause conversions to happen *before* we check whether the types are the same; is that expected? e.g., it seems like this would allow you to pass a float and a double and thanks to the magic of usual unary conversions they both come out as double and thus don't get diagnosed.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:17775-17785
+  for (int I = 1; I < 3; ++I) {
+    if (Args[0]->getType().getCanonicalType() !=
+        Args[I]->getType().getCanonicalType()) {
+      return Diag(Args[0]->getBeginLoc(),
+                  diag::err_typecheck_call_different_arg_types)
+             << Args[0]->getType() << Args[I]->getType();
+    }
----------------
These two loops can be merged together, right?


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

https://reviews.llvm.org/D140992