fhahn marked 5 inline comments as done.
fhahn added a comment.
Thanks for taking a look!
================
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))
{
----------------
aaron.ballman wrote:
> `Sema` typically comes first, so this is just to match local style.
Thanks, adjusted!
================
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;
----------------
aaron.ballman wrote:
> 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?
The builtins as implemented accept either vector types or a scalar type, which
then must be a valid element type for matrix types. The second check may be a
bit confusing, but the restrictions laid out in the spec for
scalar/element-types match the matrix element type restrictions, so this seems
a convenient helper to use.
Does this in general make sense?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16669
+ExprResult Sema::SemaBuiltinElementwiseMath(CallExpr *TheCall,
+ ExprResult CallResult) {
+ if (checkArgCount(*this, TheCall, 2))
----------------
aaron.ballman wrote:
> Do we actually need this parameter?
No, it can just return `ExprResult(TheCall)` instead I think!
================
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)
----------------
aaron.ballman wrote:
> Should these arguments undergo usual conversions (array to pointer decay,
> integer promotions, etc)?
I intentionally went with not performing conversions, because it might lead to
surprising results. If the arguments have different types, it is not clear to
me which type should be chosen to try convert the other one; e.g. if we have
__builtin_elementwise_max(float, int)` should the first argument be converted
to `int` or the second one to `float`?
Forcing the types to match without conversion seem to make them less
error-prone to use, but I might be missing some general rule to handle type
conflicts here?
================
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}}
+
----------------
aaron.ballman wrote:
> Also:
> ```
> i = __builtin_elementwise_max(i, i, i); // too many arguments
> ```
I'll add it
================
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)}}
+}
----------------
aaron.ballman wrote:
> 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)?
> ```
Thanks I'll add those!
at the moment `__builtin_elementwise_min(i, s); // ok (integer promotions)?`
would be rejected, as per my response in Sema.
The other currently are not handled properly, which I'll fix!
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
