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
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
