[PATCH] D111985: [Clang] Add elementwise min/max builtins.

[Florian Hahn via Phabricator via cfe-commits]

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