From owner-svn-src-head@FreeBSD.ORG Sat Aug 21 10:27:31 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 262731065672; Sat, 21 Aug 2010 10:27:31 +0000 (UTC) (envelope-from rpaulo@freebsd.org) Received: from karen.lavabit.com (karen.lavabit.com [72.249.41.33]) by mx1.freebsd.org (Postfix) with ESMTP id DDFDF8FC15; Sat, 21 Aug 2010 10:27:30 +0000 (UTC) Received: from d.earth.lavabit.com (d.earth.lavabit.com [192.168.111.13]) by karen.lavabit.com (Postfix) with ESMTP id 0E01911BAD2; Sat, 21 Aug 2010 05:27:30 -0500 (CDT) Received: from 10.0.10.3 (54.81.54.77.rev.vodafone.pt [77.54.81.54]) by lavabit.com with ESMTP id NS08RJRZN0F4; Sat, 21 Aug 2010 05:27:30 -0500 Mime-Version: 1.0 (Apple Message framework v1081) Content-Type: text/plain; charset=us-ascii From: Rui Paulo In-Reply-To: <20100821054033.M19850@delplex.bde.org> Date: Sat, 21 Aug 2010 11:27:26 +0100 Content-Transfer-Encoding: quoted-printable Message-Id: <91DC7F07-404F-4F9F-973D-682F97425DD2@FreeBSD.org> References: <201008191259.o7JCxv3i004613@svn.freebsd.org> <20100820075236.L18914@delplex.bde.org> <4C6DC0F8.9040001@andric.com> <20100821054033.M19850@delplex.bde.org> To: Bruce Evans X-Mailer: Apple Mail (2.1081) Cc: svn-src-head@FreeBSD.org, Dimitry Andric , src-committers@FreeBSD.org, svn-src-all@FreeBSD.org 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: Sat, 21 Aug 2010 10:27:31 -0000 On 20 Aug 2010, at 21:36, Bruce Evans wrote: > On Fri, 20 Aug 2010, Dimitry Andric wrote: >=20 >> 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... >>=20 >> 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? >=20 > 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. I think he meant that downcasting might not work. >=20 >>>> which are apparently "heinous" GNU extensions, so clang can >>>> compile this without using the -fheinous-gnu-extensions option. >>>=20 >>> But when the args are lvalues, casting them is invalid. This is >>> apparently the heinous extension depended on. >>=20 >> Yes, clang complains precisely about that: >>=20 >> 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 =3D {.ll =3D __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); = \ >> ^ >>=20 >> 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. >=20 > I prefer this. I'll commit this. >=20 >>>> Results in *no* binary change, neither with clang, nor with gcc. >>>=20 >>> 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. >>=20 >> 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. >=20 > 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. >=20 > 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. =46rom your patch: >=20 > % #define count_leading_zeros(count, x) \ > % do { = \ > % USItype __cbtmp; = \ > % __asm__ ("bsrl %1,%0" = \ > % - : "=3Dr" (__cbtmp) : "rm" (x)); = \ > % + : "=3Dr" (__cbtmp) : "rm" ((USItype) (x))); = \ > % (count) =3D __cbtmp ^ 31; = \ > % } while (0) > % #define count_trailing_zeros(count, x) \ >=20 > 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 =3D=3D= 32 > ifdef, which makes UWtype the same width as USItype so the cast has no > effect if x has the correct type. >=20 > 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). I agree with you. Thanks for the careful analysis. Regards, -- Rui Paulo