From owner-svn-src-all@freebsd.org Mon May 30 06:36:39 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E57EAB54E44 for ; Mon, 30 May 2016 06:36:39 +0000 (UTC) (envelope-from mailing-machine@vniz.net) Received: from mail-lf0-f53.google.com (mail-lf0-f53.google.com [209.85.215.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 7576E1733 for ; Mon, 30 May 2016 06:36:39 +0000 (UTC) (envelope-from mailing-machine@vniz.net) Received: by mail-lf0-f53.google.com with SMTP id s64so44145801lfe.0 for ; Sun, 29 May 2016 23:36:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=l3LfpkoOsV7MzQ/bOYIIOcL3/EWIgWG9klD6f7jjQh8=; b=S11CiqltcT6fw5Zr3i6XkMYL0BRKJf/9saGcrhdJHXxRo1PKlx9D3AyhDEoCKK2p77 yhpFyZlJCYX5wEOkUTPfOCAyZq9IB+PmwfNllmFlima8HUNpkcHId0GCSIhwO23T9YtF /0+myDXM8y0tkqsa5j8RIfdkplS/gvTmR5fm+PZ8JBXBQm2qH1R4VVIxlxT9VbHSRfvt DMMn8MKqdQpOLN6DVJBJAbi0EXlR4p9IJXw7GU8KtYS3w8F6YFIvnilKMc7jPu/tUQLi QDjNxTS0KMLNKKZMUoXVbNLb7ls31AWESQrzRhKhKM2+2pOgdB5EHg2ONdY6u2Nx0HzP 2U1Q== X-Gm-Message-State: ALyK8tKKpW9j6p7A9qEsxJFCEQo0utd8uqOuQ2+AX3w0CUhpr3v5pzMwYLbB0DifytRPrw== X-Received: by 10.25.26.18 with SMTP id a18mr7809126lfa.219.1464590196879; Sun, 29 May 2016 23:36:36 -0700 (PDT) Received: from [192.168.1.2] ([89.169.173.68]) by smtp.gmail.com with ESMTPSA id g69sm2126158ljg.25.2016.05.29.23.36.35 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 May 2016 23:36:36 -0700 (PDT) Subject: Re: svn commit: r300965 - head/lib/libc/stdlib To: Bruce Evans , Conrad Meyer References: <201605291639.u4TGdSwq032144@repo.freebsd.org> <20160530122100.X924@besplex.bde.org> Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org From: Andrey Chernov Message-ID: <5985bdc1-b821-f352-0bc5-c45c600c5318@freebsd.org> Date: Mon, 30 May 2016 09:36:34 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160530122100.X924@besplex.bde.org> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 May 2016 06:36:40 -0000 On 30.05.2016 6:09, Bruce Evans wrote: > On Sun, 29 May 2016, Conrad Meyer wrote: > >> Does clang actually generate different code with this change? > > It should, on exotic arches. > >> On Sun, May 29, 2016 at 9:39 AM, Andrey A. Chernov >> wrote: >>> Log: >>> Micro optimize: C standard guarantees that right shift for unsigned >>> value >>> fills left bits with zero, and we have exact 32bit unsigned value >>> (uint32_t), so there is no reason to add "& 0x7fffffff" here. > > Using uint32_t at all is an unportable pessimization. On exotic arches > that don't have native uint32_t registers, a theoretically perfect > implementation wouldn't implement uint32_t. This would expose the > brokenness of broken code, so a practical implementation would emulate > uint32_t so that the broken code would just run slower for arithmetic > and much slower for locked memory operations. > > uint32_t might be implemented not very slowly using 128-bit integer > registers, or more slowly using the 53-bit mantissa part of an IEEE > double precision floating point register. > > If uint32_t is emulated, then the compiler is forced to act as if the > code uses a longer type and does "& 0xffffffff" after every operation. > Thes extra operations can only be combined sometimes. More careful > code can use a minimal number of this or similar "&" operations. In > checksum calculations, one "&" at the end is usually enough. > >>> Modified: head/lib/libc/stdlib/random.c > > random.c was mostly written before uint32_t was standard, so it used > u_long and long. Perhaps it wasn't careful enough with the "&"s to > actually work unless u_long is precisely uint32_t and long is normal > 2's complement with benign overflow. > > Anyway, it was "fixed" (unimproved) using s/u_long/uint32_/ in most > places where the API/ABI doesn't require longs (there is 1 dubious > long left in a comment). The correct fix is s/u_long/uint_fast32_t > in most places and s/u_long/uint_least32_t/ in some places and then > fix any missing "&"'s. The "fast" and "least" types always exist, > unlike the fixed-width types, and using them asks for time/space > efficiency instead of emulated fixed-width. > > On non-exotic arches, fast == least == fixed-width, so the correct > substitution works as a quick fix even with missing "&"s. > > It is not necessary to use the newfangled standard integer types to > fix this here, since correct use of long types would work (they give > 32 bits), but long is wasteful if it actually 64 bits or longer. > > Even larger problems are looming with uintmax_t. Any code that is > careful enough to use it is likely to break or be bloated if it is > expanded. This is just like using u_long in old random(). > >>> ============================================================================== >>> >>> --- head/lib/libc/stdlib/random.c Sun May 29 16:32:56 >>> 2016 (r300964) >>> +++ head/lib/libc/stdlib/random.c Sun May 29 16:39:28 >>> 2016 (r300965) >>> @@ -430,7 +430,7 @@ random(void) >>> */ >>> f = fptr; r = rptr; >>> *f += *r; >>> - i = (*f >> 1) & 0x7fffffff; /* chucking least >>> random bit */ >>> + i = *f >> 1; /* chucking least random bit */ > > This gives an "&" to restore in the version with correct substitutions. > > It also breaks the indentation. (This file mostly indents comments to the > right of code to column 40, but column 48 was used here and now column 32 > is used.) > >>> if (++f >= end_ptr) { >>> f = state; >>> ++r; > > Bruce > I don't introduce uint32_t and int32_t here and don't have a slightest idea of which types will be better to change them. F.e. *f += *r; suppose unsigned 32bit overflow which don't naturally happens for large types. Assigning uint32_t to some large type then clip it to smaller after calculation - all of that can produce more code than save for calculation itself.