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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16669
+ExprResult Sema::SemaBuiltinElementwiseMath(CallExpr *TheCall,
+                                            ExprResult CallResult) {
+  if (checkArgCount(*this, TheCall, 2))
----------------
aaron.ballman wrote:
> 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.
updated to return a bool


================
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:
> fhahn wrote:
> > aaron.ballman wrote:
> > > 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).
> > > 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.
> > 
> > Yes, they should ideally work with literals! I think it should be relatively straight-forward to add promotion of literals.
> > 
> > > 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.
> > 
> > IIUC this happens in `SemaOverload.cpp`, but I am not sure how builtins would hook into the infrastructure there. Are there other builtins that are similarly overloaded by any chance?
> > Yes, they should ideally work with literals! I think it should be relatively straight-forward to add promotion of literals.
> 
> I was thinking of the literal being an `int` and the variable being a `short`, so the literal isn't what needs promotion in that case. That said, I suppose character literals in C++ would promote from `char` to `int`, so there are some literals that could promote. That'd be a somewhat amusing test case for C and C++ both (`__builtin_elementwise_max('a', 1);`).
> 
> > IUC this happens in SemaOverload.cpp, but I am not sure how builtins would hook into the infrastructure there. Are there other builtins that are similarly overloaded by any chance?
> 
> `Sema::SemaBuiltinUnorderedCompare()` uses this, for example:
> ```
>   ExprResult OrigArg0 = TheCall->getArg(0);
>   ExprResult OrigArg1 = TheCall->getArg(1);
> 
>   // Do standard promotions between the two arguments, returning their common
>   // type.
>   QualType Res = UsualArithmeticConversions(
>       OrigArg0, OrigArg1, TheCall->getExprLoc(), ACK_Comparison);
>   if (OrigArg0.isInvalid() || OrigArg1.isInvalid())
>     return true;
> ```
> hat'd be a somewhat amusing test case for C and C++ both (__builtin_elementwise_max('a', 1);).

I added ` i1 = __builtin_elementwise_max(1, 'a');` to the codegen tests.

> Sema::SemaBuiltinUnorderedCompare() uses this, for example:

Thanks a lot! I updated to code to use `UsualArithmeticConversions` and added the tests you suggested (hope I didn't miss any important ones). Looks like all should work as expected now.


================
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:
> aaron.ballman wrote:
> > 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)?
> > ```
> You might also need to handle something like:
> ```
> int (i);
> int j;
> __builtin_elementwise_min(i, j);  // May get a type mismatch here
> ```
> So you may need to do some type canonicalization to avoid these sorts of issues.
Thanks, I updated the code to check the canonical type. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111985