Hey Renato, thanks very much for taking another look. In http://reviews.llvm.org/D9913#192654, @rengolin wrote:
> From what I read here [1], these builtins can be compiler replacements for > library functions, especially if they have the "__builtin" prefix: Right. There are still some omissions in what Clang marks up a library builtin and what GCC does outside strict ISO C mode. You already pointed out the `signbit` omissions, but here are all the ones I still haven't marked up after this patch BUILTIN(__builtin_signbitf, "if", "nc") BUILTIN(__builtin_signbitl, "iLd", "nc") BUILTIN(__builtin_ffsll, "iLLi", "nc") BUILTIN(__builtin_bcmp, "iv*v*z", "n") BUILTIN(__builtin_alloca, "v*z" , "n") I wasn't sure if Clang would be interested in compatibility in this case. So I originally focused on the C99 floating point stuff that is part of a standard I could read. Do you think I should patch up the above missing bits too? > > > "... may be handled as built-in functions. All these functions have > corresponding versions prefixed with __builtin_ ..." > > > And from the bug report [2], what Android needs is for it to be re-declared > differently, and that's why you're adding them as library calls, so that can > happen. That's basically it. These C99 floating-point functions are defined as taking either a float, a double or a long double as argument(s). Operationally, we don't want to commit to declaring one of these options too soon, since the user (as in the Android case) might want to redeclare it in an incompatible (but legal) way later. What Clang currently does is declare the type-generic builtins like this with an empty parameter list, which gives weird behaviour in both C and C++ when you try to redeclare them. I'm marking them up as library calls so that I can selectively handle just the builtins I need to in my patch in http://reviews.llvm.org/D9912. > Is then, this patch ultimately linked to http://reviews.llvm.org/D9912? Or > does this make sense on its own? If it does, I'm failing to see why. :) I tried to mention that with my link in Phabricator, but I realize I've failed to make it clear. There doesn't seem to be a good way of posting a logical patch set to Phab. Aside from http://reviews.llvm.org/D9912 relying on this change, I also think it's fine to go in on its own, since these builtins should be marked up as "F", given that they're listed in the C99 standard. Do you agree? Thanks again for your time, - Charlie. > [1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html > [2] https://llvm.org/bugs/show_bug.cgi?id=20958 http://reviews.llvm.org/D9913 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
