On 06/02/2016 01:34 AM, Jakub Jelinek wrote:
On Thu, Jun 02, 2016 at 09:23:16AM +0200, Jakub Jelinek wrote:Also, perhaps just a documentation thing, it would be good to clarify the NULL last argument. From the POV of generating efficient code, I think we should say something that the last argument (for the GNU builtins) must be either a pointer to a valid object, or NULL/nullptr constant cast expression cast to a pointer type, but nothing else. That is actually what your patch implements. But, I'd like to make sure that int *p = NULL; if (__builtin_add_overflow (a, b, p)) ... is actually not valid, otherwise we unnecessarily pessimize many of the GNU style calls (those that don't pass &var), where instead of tem = ADD_OVERFLOW (a, b); *p = REALPART_EXPR (tem); ovf = IMAGPART_EXPR (tem); we'd need to emit instead tem = ADD_OVERFLOW (a, b); ovf = IMAGPART_EXPR (tem); if (p != NULL) *p = REALPART_EXPR (tem);Though, perhaps that is too ugly, that it has different semantics for __builtin_add_overflow (a, b, (int *) NULL) and for int *p = NULL; __builtin_add_overflow (a, b, p) Maybe the cleanest would be to just add 3 extra builtins, again, typegeneric, __builtin_{add,sub,mul}_overflow_p where either the arguments would be instead of integertype1, integertype2, integertype3 * rather integertype1, integertype2, integertype3 and we'd only care about the type, not value, of the last argument, so use it like __builtin_add_overflow_p (a, b, (__typeof ((a) + (b))) 0) or handle those 3 similarly to __builtin_va_arg, and use __builtin_add_overflow_p (a, b, int); I think I prefer the former though.
I'm not sure I understand your concern but at this point I would prefer to keep things as they are. I like that the functionality that was requested in c/68120 could be provided under the existing interface, and I'm not fond of the idea of adding yet another set of built-ins just to get at a bit that's already available via the existing ones (in C++ 11, even in constexpr contexts, provided the built-ins were allowed there). I've also spent far more time on this patch than I had planned and I'm way behind on the tasks I was asked to work on. That said, in c/68120 the requester commented that he's considering submitting a request for yet another enhancement in this area, so I think letting him play with what we've got now for a bit will be an opportunity to get his feedback and tweak the API further based on it. Martin
