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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 6 inline comments as done.
fhahn 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)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111985