[PATCH] D140992: clang: Add __builtin_elementwise_fma

[Matt Arsenault via Phabricator via cfe-commits]

arsenm marked an inline comment as done.
arsenm added inline comments.


================
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))
----------------
aaron.ballman wrote:
> 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.
There already are complex tests for these. For some reason the type check was split between here and checkMathBuiltinElementType through PrepareBuiltinElementwiseMathOneArgCall


================
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();
+  }
----------------
aaron.ballman wrote:
> 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.
The tests say that isn't happening, e.g. here:


```

  f32 = __builtin_elementwise_fma(f64, f32, f32);
  // expected-error at -1 {{arguments are of different types ('double' vs 'float')}}

  f32 = __builtin_elementwise_fma(f32, f64, f32);
  // expected-error at -1 {{arguments are of different types ('float' vs 'double')}}

```


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

https://reviews.llvm.org/D140992