fhahn marked 3 inline comments as done.
fhahn added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8747-8748
+def err_elementwise_math_arg_types_mismatch : Error <
+ "argument types do not match, %0 != %1">;
+
----------------
aaron.ballman wrote:
> Does `err_typecheck_call_different_arg_types` suffice?
`err_typecheck_call_different_arg_types` is sufficient, thanks!
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8750-8751
+
+def err_elementwise_math_invalid_arg_type: Error <
+ "argument type %0 is not supported">;
+
----------------
aaron.ballman wrote:
> It'd be nice to avoid this entirely as the diagnostic situation is one we
> would have elsewhere (so we should be able to use common diagnostic checking
> code from here and places like `SemaBuiltinMatrixColumnMajorLoad()` ideally).
I turned the diagnostic kind into a more generic one and also put up a patch to
use them instead some of the custom matrix diagnostic kinds (D112532). WDYT?
I'm not sure if there's much potential for sharing the checking code, as it
looks like most places check for slightly different things (or the check is
quite compact already).
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16662-16667
+ if (!Ty->getAs<VectorType>() && !ConstantMatrixType::isValidElementType(Ty))
{
+ S.Diag(Loc, diag::err_elementwise_math_invalid_arg_type) << Ty;
+ return true;
+ }
+ return false;
+}
----------------
aaron.ballman wrote:
> Related to the comment above on the diagnostic, I'm wondering if we want to
> abstract this into a helper function that gets used by all the elementwise
> builtins you're adding?
Unfortunately I am not sure which code this is referring to. The element type
check is already a generic function to be used to check the new builtins with 1
arg and the reduction builtins.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:16695
+ TheCall->setArg(1, B.get());
+ TheCall->setType(TyB);
+ return false;
----------------
aaron.ballman wrote:
> I think you want to set this to `Res`, because that's the common type between
> `TyB` and `TyA`, right? That will also ensure that qualifiers are stripped, I
> believe. e.g.,
> ```
> const int a = 2;
> int b = 1;
> static_assert(!std::is_const_v<decltype(__builtin_elementwise_max(a, b))>);
> static_assert(!std::is_const_v<decltype(__builtin_elementwise_max(b, a))>);
> ```
Agreed, using `TyB` is confusing, so I updated it. I *think* it was still doing
the right thing, because `TyB` is to `B`'s type after conversion, so I *think*
that would be to common type already.
I also added a `.cpp` Sema test file to check the constness.
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
