[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

1 nit, and 1 trying to see what is going on.  I don't have a good feeling what the purpose of this builtin is, nor whether it matches the desire/intent of this builtin, I'm hopeful one of the other reviewers has the ability to check that.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:2048
+  if (!EltTy->isRealFloatingType()) {
+    S.Diag(Loc, diag::err_builtin_invalid_arg_type)
+        << ArgIndex << /* vector or float ty*/ 5 << ArgTy;
----------------
you can just do "return S.Diag", which always returns 'true'.  This will save a line, and the need for curleys.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:2660
+
+    ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0));
+    ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1));
----------------
What is the point of the Unary Conversions here?  Its a touch surprising to see a builtin do that?  Note that it does quite a bit of FP related conversions, so are you sure you want those?


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

https://reviews.llvm.org/D140639