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

[Florian Hahn via Phabricator via cfe-commits]

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