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@-1 {{arguments are of different types ('double' vs
'float')}}
f32 = __builtin_elementwise_fma(f32, f64, f32);
// expected-error@-1 {{arguments are of different types ('float' vs
'double')}}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140992/new/
https://reviews.llvm.org/D140992
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
