From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13053 invoked by alias); 21 Aug 2014 21:23:33 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 13043 invoked by uid 89); 21 Aug 2014 21:23:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.9 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 21 Aug 2014 21:23:31 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7LLNSGd013294 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 21 Aug 2014 17:23:28 -0400 Received: from greed.delorie.com (ovpn-113-60.phx2.redhat.com [10.3.113.60]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s7LLNRIJ025060 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 21 Aug 2014 17:23:28 -0400 Received: from greed.delorie.com (greed.delorie.com [127.0.0.1]) by greed.delorie.com (8.14.4/8.14.4) with ESMTP id s7LLNQ8U018747; Thu, 21 Aug 2014 17:23:26 -0400 Received: (from dj@localhost) by greed.delorie.com (8.14.4/8.14.4/Submit) id s7LLNPIQ018746; Thu, 21 Aug 2014 17:23:25 -0400 Date: Thu, 21 Aug 2014 21:23:00 -0000 Message-Id: <201408212123.s7LLNPIQ018746@greed.delorie.com> From: DJ Delorie To: "Joseph S. Myers" CC: gcc-patches@gcc.gnu.org In-reply-to: (joseph@codesourcery.com) Subject: Re: __intN patch 3/5: main __int128 -> __intN conversion. References: <201408132211.s7DMBGBu016387@greed.delorie.com> X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg02145.txt.bz2 > Why are types only entered in integer_types if wider than long long? IIRC it was so that TImode (__int128) could get into the array (it was there before) without adding the other smaller types, which (I think) don't need to be in there. I don't recall why they're left out, but... ah, I remember. ITK has to be sorted by bitsize and it's used for type promotion, we don't want types promoted *to* the __intN types, just *from* them. > > +static bool > > +standard_type_bitsize (int bitsize) > > +{ > > + /* As a special exception, we always want __int128 enabled if possible. */ > > + if (bitsize == 128) > > + return false; > > + if (bitsize == CHAR_TYPE_SIZE > > + || bitsize == SHORT_TYPE_SIZE > > + || bitsize == INT_TYPE_SIZE > > + || bitsize == LONG_TYPE_SIZE > > + || (bitsize == LONG_LONG_TYPE_SIZE && LONG_LONG_TYPE_SIZE < GET_MODE_BITSIZE (TImode))) > > I don't think there should be a special case depending on the size of long > long here. I think it was from before I had the special case for "bitsize == 128". I'll remove it. > > + /* This must happen after the backend has a chance to process > > + command line options, but before the parsers are > > + initialized. */ > > + for (i = 0; i < NUM_INT_N_ENTS; i ++) > > + if (targetm.scalar_mode_supported_p (int_n_data[i].m) > > + && ! standard_type_bitsize (int_n_data[i].bitsize) > > + && int_n_data[i].bitsize <= HOST_BITS_PER_WIDE_INT * 2) > > + int_n_enabled_p[i] = true; > > This HOST_BITS_PER_WIDE_INT * 2 check seems wrong. That check was there before, for __int128, I left it as-is. There is no __int128 (currently) if it's bigger then HBPWI*2. > > mode_for_size (unsigned int size, enum mode_class mclass, int limit) > > { > > - enum machine_mode mode; > > + enum machine_mode mode = BLKmode; > > + int i; > > I don't see any need for this initialization to be added. Removed. > > bprecision > > - = MIN (precision + BITS_PER_UNIT_LOG + 1, MAX_FIXED_MODE_SIZE); > > + = MIN (precision, MAX_FIXED_MODE_SIZE); > > This change doesn't make sense to me - if it is needed, I think it should > be separated out, not included in this patch which should simply about > providing generic __intN support in the cases where __int128 is currently > supported. Perhaps is belongs in the 1/5 patch, where I removed lots of "assume all types are powers-of-two sizes" logic? I split up the patch and logs by hand from a master patch, I'm not surprised I got a few mis-placed. > > @@ -1157,13 +1165,13 @@ enum omp_clause_map_kind > > OMP_CLAUSE_MAP_ALLOC, > > OMP_CLAUSE_MAP_TO, > > OMP_CLAUSE_MAP_FROM, > > OMP_CLAUSE_MAP_TOFROM, > > /* The following kind is an internal only map kind, used for pointer based > > array sections. OMP_CLAUSE_SIZE for these is not the pointer size, > > - which is implicitly POINTER_SIZE / BITS_PER_UNIT, but the bias. */ > > + which is implicitly POINTER_SIZE_UNITS, but the bias. */ > > POINTER_SIZE_UNITS changes belong in another patch, not this one. Again, probably the 1/5 patch. > > /* ptr_type_node can't be used here since ptr_mode is only set when > > toplev calls backend_init which is not done with -E or pch. */ > > - psize = POINTER_SIZE / BITS_PER_UNIT; > > + psize = POINTER_SIZE_UNITS; > > Likewise. Likewise :-) > > @@ -844,12 +846,47 @@ c_cpp_builtins (cpp_reader *pfile) > > builtin_define_type_max ("__LONG_LONG_MAX__", long_long_integer_type_node); > > builtin_define_type_minmax ("__WCHAR_MIN__", "__WCHAR_MAX__", > > underlying_wchar_type_node); > > builtin_define_type_minmax ("__WINT_MIN__", "__WINT_MAX__", wint_type_node); > > builtin_define_type_max ("__PTRDIFF_MAX__", ptrdiff_type_node); > > builtin_define_type_max ("__SIZE_MAX__", size_type_node); > > + for (i = 0; i < NUM_INT_N_ENTS; i ++) > > + if (int_n_enabled_p[i]) > > + { > > + char buf[35+20+20]; > > + char buf_min[15+20]; > > + char buf_max[15+20]; > > + > > + sprintf(buf_min, "__INT%d_MIN__", int_n_data[i].bitsize); > > + sprintf(buf_max, "__INT%d_MAX__", int_n_data[i].bitsize); > > All this block of code appears to be new rather than replacing any > existing code doing something similar with __int128. As such, I think > it's best considered separately from the main __intN support. For each __int we need to provide an __INT_MIN__ and __INT_MAX__, just like for "char" we provide __CHAR_MIN__ and __CHAR_MAX__. > Also note missing spaces before '(' in this code. Fixed. > Some of this may be needed for libstdc++, but not all. As far as I can > tell, the existing __glibcxx_min, __glibcxx_max, __glibcxx_digits, > __glibcxx_digits10, __glibcxx_max_digits10 macros can be used in most > cases and avoid any need to predefine macros for the min or max of __intN; > you only need to communicate which types exist and their sizes in bits > (that is, a single macro predefine for each N, with anything else being > considered separately if otherwise thought desirable). I tried that, and wasn't able to get a simpler macro to do it inline than the full macro that let gcc figure out the values. Consider the two N of 20 and 128; one is not a multiple of bytes and the other will likely stress any runtime math. > > + if (!in_system_header_at (input_location) > > + /* As a special exception, allow a type that's used > > + for __SIZE_TYPE__. */ > > + && int_n_data[specs->int_n_idx].bitsize != POINTER_SIZE) > > pedwarn (loc, OPT_Wpedantic, > > - "ISO C does not support %<__int128%> type"); > > + "ISO C does not support %<__int%d%> types", > > + int_n_data[specs->int_n_idx].bitsize); > > I don't think there should be such a special exception. Existing practice > is for testcases to do "__extension__ typedef __SIZE_TYPE__ size_t;" to > avoid -pedantic diagnostics for size_t being unsigned long long on Win64. OK, I can do that instead. There was some discussion about how to handle size_t without warnings when it corresponded to an extension type, and I thought the general consensus was to skip the warning rather than break the code. Note that the C++ libraries must refer to __intN throughout as it's a core type, they can't use size_t because that's a typedef. I.e. even code that specifically uses "size_t" will end up using the <__int20> templates. Hopefully the "in system header" test will be enough to handle uses inside the testsuite. > > - align = exact_log2 (POINTER_SIZE / BITS_PER_UNIT); > > + align = ceil_log2 (POINTER_SIZE_UNITS); > > Again, belongs in a different patch. patch 1/5.