[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: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:
> > 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;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111985