aaron.ballman added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:2660 + + ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0)); + ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1)); ---------------- 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()`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140639/new/ https://reviews.llvm.org/D140639 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
