aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:12715-12716
+ ExprResult SemaBuiltinElementwiseMath(CallExpr *TheCall,
+ ExprResult CallResult);
+
----------------
<nothing to fix>Why oh why did we start slapping `Sema` as a prefix on a bunch
of functions implemented in a class named `Sema`?</nothing to fix>
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1981
return SemaBuiltinMatrixTranspose(TheCall, TheCallResult);
-
case Builtin::BI__builtin_matrix_column_major_load:
----------------
Spurious whitespace change?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16659-16660
+// false.
+static bool checkMathBuiltinElementType(SourceLocation Loc, QualType Ty,
+ Sema &S) {
+ if (!Ty->getAs<VectorType>() && !ConstantMatrixType::isValidElementType(Ty))
{
----------------
`Sema` typically comes first, so this is just to match local style.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16661
+ Sema &S) {
+ if (!Ty->getAs<VectorType>() && !ConstantMatrixType::isValidElementType(Ty))
{
+ S.Diag(Loc, diag::err_elementwise_math_invalid_arg_type) << Ty;
----------------
I'm a bit surprised we'd need `!Ty->getAs<VectorType>()` as I would have
expected `!ConstantMatrixType::isValidElementType(Ty)` to cover all the type
checking of `Ty`. Can you explain why the extra check is needed here?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16669
+ExprResult Sema::SemaBuiltinElementwiseMath(CallExpr *TheCall,
+ ExprResult CallResult) {
+ if (checkArgCount(*this, TheCall, 2))
----------------
Do we actually need this parameter?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16673-16678
+ Expr *A = TheCall->getArg(0);
+ Expr *B = TheCall->getArg(1);
+ QualType TyA = A->getType();
+ QualType TyB = B->getType();
+
+ if (TyA != TyB)
----------------
Should these arguments undergo usual conversions (array to pointer decay,
integer promotions, etc)?
================
Comment at: clang/test/Sema/builtins-elementwise-math.c:20-21
+
+ i = __builtin_elementwise_max();
+ // expected-error@-1 {{too few arguments to function call, expected 2, have
0}}
+
----------------
Also:
```
i = __builtin_elementwise_max(i, i, i); // too many arguments
```
================
Comment at: clang/test/Sema/builtins-elementwise-math.c:42
+ // expected-error@-1 {{argument types do not match, 'float4' (vector of 4
'float' values) != 'int3' (vector of 3 'int' values)}}
+}
----------------
Additional tests I would like to see:
```
int i;
short s;
__builtin_elementwise_min(i, s); // ok (integer promotions)?
enum e { one, two };
__builtin_elementwise_min(one, two); // ok (using enums)?
enum f { three };
__builtin_elementwise_min(one, three); // ok (different enums)?
_ExtInt(32) ext;
__builtin_elementwise_min(ext, ext); // ok (using bit-precise integers)?
const int ci;
__builtin_elementwise_min(i, ci); // ok (qualifiers don't match)?
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111985/new/
https://reviews.llvm.org/D111985
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
