From owner-svn-src-head@FreeBSD.ORG Fri Aug 20 20:37:18 2010 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6782F10656A7; Fri, 20 Aug 2010 20:37:18 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id F3E9F8FC1D; Fri, 20 Aug 2010 20:37:17 +0000 (UTC) Received: from c122-107-127-123.carlnfd1.nsw.optusnet.com.au (c122-107-127-123.carlnfd1.nsw.optusnet.com.au [122.107.127.123]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o7KKakCg007642 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 21 Aug 2010 06:36:48 +1000 Date: Sat, 21 Aug 2010 06:36:45 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Dimitry Andric In-Reply-To: <4C6DC0F8.9040001@andric.com> Message-ID: <20100821054033.M19850@delplex.bde.org> References: <201008191259.o7JCxv3i004613@svn.freebsd.org> <20100820075236.L18914@delplex.bde.org> <4C6DC0F8.9040001@andric.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Rui Paulo , Bruce Evans Subject: Re: svn commit: r211505 - head/contrib/gcc X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Aug 2010 20:37:18 -0000 On Fri, 20 Aug 2010, Dimitry Andric wrote: > On 2010-08-20 00:20, Bruce Evans wrote: >> These seem to be needed, and some of them valid. Any lvalue arg that >> can be put in a register can be cast to USItype (unsigned int) on i386. >> The args are macro args, so they may have any integer type no larger >> than USItype originally, and they must be cast to USItype for the asms >> to work if the args are smaller than USItype... > > But will the casts not potentially hide problems, if you pass the wrong > types to those macros? Maybe it is better if the compiler complains > that some argument is of an incompatible type, than just forcing it to > cast? This is unclear. All integer types are compatible to some extent. Upcasting them always works and downcasting them works iff the value is not changed. >>> which are apparently "heinous" GNU extensions, so clang can >>> compile this without using the -fheinous-gnu-extensions option. >> >> But when the args are lvalues, casting them is invalid. This is >> apparently the heinous extension depended on. > > Yes, clang complains precisely about that: > > gnu/lib/libgcc/../../../contrib/gcc/libgcc2.c:536:22: error: invalid use of a cast in a inline asm context requiring an l-value: remove the cast or build with -fheinous-gnu-extensions > DWunion w = {.ll = __umulsidi3 (uu.s.low, vv.s.low)}; > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > In file included from gnu/lib/libgcc/../../../contrib/gcc/libgcc2.c:65: > In file included from gnu/lib/libgcc/../../../contrib/gcc/libgcc2.h:435: > gnu/lib/libgcc/../../../contrib/gcc/longlong.h:1293:5: note: instantiated from: > umul_ppmm (__w.s.high, __w.s.low, u, v); \ > ^ > > It turns out that only removing the casts for these specific lvalues is > indeed enough to make clang happy. Attached patch reverts all other > changes, if that is to be preferred. I prefer this. >>> Results in *no* binary change, neither with clang, nor with gcc. >> >> This cannot be tested by compiling a few binaries, since the few binaries >> might not include enough examples to give test coverage of cases with >> args smaller than USItype. Perhaps the macros are only used for building >> libgcc, but this is unclear. > > contrib/gcc/longlong.h is only used in contrib/gcc/libgcc2.h, and in > contrib/gcc/config/soft-fp/soft-fp.h. On i386, soft-fp is not used, > and libgcc2.h is only used in contrib/gcc/libgcc2.c, so libgcc is the > only consumer, as far as I can see. There are many parameters. Probably the casts are needed for some arches, so gnu can't just remove them and wouldn't like your patch. But they should fix the casts of lvalues someday. The ifdefs are hard to untangle. I thought I found a simple case where the cast helps, but actually the ifdefs make the cast have no effect although the code is logically wrong. From your patch: % #define count_leading_zeros(count, x) \ % do { \ % USItype __cbtmp; \ % __asm__ ("bsrl %1,%0" \ % - : "=r" (__cbtmp) : "rm" (x)); \ % + : "=r" (__cbtmp) : "rm" ((USItype) (x))); \ % (count) = __cbtmp ^ 31; \ % } while (0) % #define count_trailing_zeros(count, x) \ This interface is supposed to operate on `UWtype x'. UWtype is UDItype on some arches, so casting it to USItype breaks it. However, the breakage is only logical since all this is under a __i386__ && W_TYPE_SIZE == 32 ifdef, which makes UWtype the same width as USItype so the cast has no effect if x has the correct type. The above wouldn't work on amd64 since the correct code is bsrq with no cast (the value is 64 bits; the cast would give a 32-bit value; apart from breaking the value, this would give invalid asm like "bsrq %eax" if the casted value ends up in a register. Handling all the cases is apparently too hard, so longlong.h doesn't have any special support for clz on amd64 and a generic version -- the above is apparently used for __clzsi2() and _clzdi2() on i386, but on amd64 a slow loop is used. gcc asm still seems to be missing support for writing this correctly ("bsr%[mumble1] %1,%0", where %[mumble1] is either q or l (or w or b?) depending on the size of %1). Bruce