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>