Date: Sat, 21 Aug 2010 11:27:26 +0100 From: Rui Paulo <rpaulo@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, Dimitry Andric <dimitry@andric.com>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org Subject: Re: svn commit: r211505 - head/contrib/gcc Message-ID: <91DC7F07-404F-4F9F-973D-682F97425DD2@FreeBSD.org> In-Reply-To: <20100821054033.M19850@delplex.bde.org> References: <201008191259.o7JCxv3i004613@svn.freebsd.org> <20100820075236.L18914@delplex.bde.org> <4C6DC0F8.9040001@andric.com> <20100821054033.M19850@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?91DC7F07-404F-4F9F-973D-682F97425DD2>