svn commit: r211505 - head/contrib/gcc

[Rui Paulo]

On 20 Aug 2010, at 21:36, Bruce Evans wrote:

> 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.

I think he meant that downcasting might not work.

> 
>>>> 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.

I'll commit 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).

I agree with you. Thanks for the careful analysis.

Regards,
--
Rui Paulo