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

[Aaron Ballman via Phabricator via cfe-commits]

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 at -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 at -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