[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D140639#4028900 <https://reviews.llvm.org/D140639#4028900>, @arsenm wrote:

> In D140639#4028883 <https://reviews.llvm.org/D140639#4028883>, @erichkeane wrote:
>
>> 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,
>
> The point of every builtin is direct access to llvm intrinsics, in this case llvm.copysign. I need it on vectors and not the set of 3 scalars it handles now.

I'm unfamiliar with the semantics of that builtin other than what i can read here: https://llvm.org/docs/LangRef.html#llvm-copysign-intrinsic



================
Comment at: clang/lib/Sema/SemaChecking.cpp:2660
+
+    ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0));
+    ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1));
----------------
arsenm wrote:
> erichkeane wrote:
> > 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?
> Copied from the other elementwise intrinsics, and this is at the edges of my frontend knowledge (I guess it's to avoid qualifiers mattering?). The tests seem to behave as I expect
It really depends on what behavior you're looking to get out of this.  UnaryConversions are usually for operators, not 'function like' things, so it is a touch jarring to me.

I guess I would have expected DefaultFunctionArrayLValueConversion (which calls DefaultLValueConversion after doing array-to-pointer conversions).

If the idea is for this builtin to act more like a variadic arg, I'd expect to see DefaultArgumentPromotion.

@aaron.ballman  I think is smarter than me in regards to how these should work, so perhaps he can comment here?

I DO note one of the things that UsualUnaryConversions is doing is removing 'half' types on platforms without a 'native' half type.  I would expect those sorts of conversions wouldn't be right?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:2674
+
+    if (MagnitudeTy.getCanonicalType() != SignTy.getCanonicalType()) {
+      return Diag(Sign.get()->getBeginLoc(),
----------------
curleys not used for single-statement if-statement bodies.


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

https://reviews.llvm.org/D140639