svn commit: r211505 - head/contrib/gcc

[Bruce Evans]

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