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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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;
----------------
fhahn wrote:
> 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?
Thanks for the explanation, that makes sense.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16669
+ExprResult Sema::SemaBuiltinElementwiseMath(CallExpr *TheCall,
+                                            ExprResult CallResult) {
+  if (checkArgCount(*this, TheCall, 2))
----------------
fhahn wrote:
> aaron.ballman wrote:
> > Do we actually need this parameter?
> No, it can just return `ExprResult(TheCall)` instead I think!
Could also return a `bool` and have the caller fiddle with `ExprResult`, too. I don't have strong opinions on which way to go though.


================
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)
----------------
fhahn wrote:
> 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?
I'm not certain how these builtins are expected to be used, but if they're likely to be used with literals, I think we may want integer promotions because of that alone.

Btw, given that these only have two arguments, it seems like we could use the usual arithmetic conversions to pick a common type the same way as happens for binary operators.

If you end up not adding any conversions here, we should have a test case to cover passing in two array objects (which won't decay to pointers).


================
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)}}
+}
----------------
fhahn wrote:
> 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!
I'd still like to see the test case added so it's clear we intend to reject it. It may also be wise to add a test case involving an integer literal and a `short` variable cast to `int` to make it clear that you have to add casts sometimes to make this work.

Another interesting test is where sugars don't match. e.g.,
```
int i;
__attribute__((address_space(0))) int addr_space_i;
typedef int foo;

__builtin_elementwise_min(i, addr_space_i); // ok (attributes don't match)?
__builtin_elementwise_min(i, foo); // ok (sugar doesn't match)?
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111985/new/

https://reviews.llvm.org/D111985