[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:2660
+
+    ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0));
+    ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1));
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > 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?
> I think we need to do the usual unary conversions because that's what handles the floating point evaluation method stuff, and if I'm reading this properly, it seems that `copysign()` does perform these conversions: https://godbolt.org/z/eWjEqvvjd. I would not expect to handle this with default argument promotion given the signature of `copysign()`.
Got it, thanks for the clarification.


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

https://reviews.llvm.org/D140639