[Pixman] [PATCH] Fix pixman build with older GCC releases

[Siarhei Siamashka]

On Sun, 29 Sep 2013 17:56:46 -0400
Brad Smith <brad at comstyle.com> wrote:

> On Sat, Sep 28, 2013 at 09:54:10PM +0300, Siarhei Siamashka wrote:
> > On Sat, 28 Sep 2013 00:01:27 -0400
> > Brad Smith <brad at comstyle.com> wrote:
> > 
> > > On 16/09/13 4:25 PM, S??ren Sandmann wrote:
> > > > Brad Smith <brad at comstyle.com> writes:
> > > >
> > > >> Discussion seems to have died down. Could this please be commited?
> > > >
> > > > I didn't see any reply to Siarhei's suggestion of doing it as a
> > > > configure check. Also, you'll need to send a patch formatted with "git
> > > > format-patch", with a useful conmmit log.
> > > 
> > > I looked around at the few compilers that implement this GCC-ism other 
> > > than GCC and that's only LLVM and PCC
> > 
> > For example, you forgot about the Intel compiler. Not that it is
> > causing any problems here, but this just shows that your knowledge
> > about the use of the existing compilers all over the world is a
> > little bit incomplete.
> 
> Yes, I did not come across any indication of the Intel compiler
> supporting this built in function when searching around for any
> details regarding this function, compilers supporting it, examples
> of how other projects deal with this and so forth.

And yet another example seems to be the Pathscale compiler. There are
too many GCC-compatible compilers around, though some of them are a bit
obscure and unpopular.

> > And guess what? The other people (me included) also don't have a
> > complete understanding about all the possible system configurations
> > where pixman is used. That's why you had been politely asked to
> > provide a bit more details about why exactly you are interested
> > in GCC 3.3 support. It's simple curiosity because your original
> > commit message did not try to justify the practical usefulness
> > of the patch in a way that is clear enough for everyone. The reply
> > http://lists.freedesktop.org/archives/pixman/2013-August/002842.html
> > helped really a lot, and IMHO this should be a part of the commit
> > message.
> 
> Well for me the most important thing is ensuring pixman builds not
> to ensure it is utilizing the most efficient code and/or function with
> every possible compiler out there.

Let's look at it in another way. The compilation failure is very easy to
notice and any developer (without any special skills or experience) can
easily fix/workaround it in a matter of a few minutes because the
fallback path exists in the code. And the compilation failure points
exactly to the relevant line in the sources.

On the other hand, if the slow path is silently taken, this is the
problem, which is rather hard to detect. For example, we probably
want to also use the __builtin_clz replacement with the MSVC compiler.
Now that this issue got into the spotlight, it may be perhaps resolved
too:

http://stackoverflow.com/questions/355967/how-to-use-msvc-intrinsics-to-get-the-equivalent-of-this-gcc-code

The point is that the CLZ operation maps to just a single instruction
in the vast majority of modern processors. But it can't be efficiently
expressed in the C language, so the intrinsics exist for this purpose.

> > > and they were already covered by the code as it is at the moment.
> > 
> > That's good. And such extra details (the exact GCC version these
> > compilers pretend to emulate) would be also a nice addition to
> > the commit message.
> 
> Ok, I can do that.

Thanks, that's appreciated.

> > > I don't see any reason to further complicate the diff.
> > 
> > Are you interested in fixing the problem? Or in having your
> > patch applied exactly in the way you see reasonable?
> 
> Well my diff is fixing what I consider is the immediate problem
> and that is that pixman is not building, not that it isn't using
> the most efficient resulting code with all compilers.
> 
> We obviously have different perspectives as to what the "problem"
> is.

Yep, that's true. And I have explained my perspective a few
paragraphs above.

> > I would like to get this resolved, just because I'm the one who
> > introduced this regression (by relying on the __GNUC__ ifdef and
> > incorrectly assuming that __builtin_clz has been always supported
> > by GCC). Sorry for this.
> > 
> > But your solution does not sit well with me because it adds
> > even more of the fragile ifdeffery magic. What if somebody comes
> > tomorrow, complaining about yet another problem with this ill-fated
> > ifdef and suggesting to also add some check, based on something like
> > "defined(__FOOBAR_COMPILER__)" to it?
> 
> It's not that I was completely ruling out going down the autoconf
> test way of fixing this. I just wasn't aware of any other compiler
> that implemented this functionality that was not GCC or a compiler
> essentially pretending to be GCC. I went looking around and almost
> all other projects I could find using this function uses the same
> method of detecting and using this function.

If the other projects are solving it this way, then the commit
message could also mention it. That the whole point. If you provide
a descriptive commit message, it saves time of the people trying to
review the patch and may change their opinion in your favour.

> Ok before I post an official patch so to speak I'd like some review
> of what I have come up with...
> 
> 
> diff --git a/configure.ac b/configure.ac
> index 263c63e..a269f21 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1036,6 +1036,22 @@ fi
>  
>  AC_MSG_RESULT($support_for_float128)
>  
> +dnl =====================================
> +dnl __builtin_clz
> +
> +support_for_builtin_clz=no
> +
> +AC_MSG_CHECKING(for __builtin_clz)
> +AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
> +unsigned int x = 11; int main (void) { __builtin_clz(x); }
> +]])], support_for_builtin_clz=yes)
> +
> +if test x$support_for_builtin_clz = xyes; then
> +   AC_DEFINE([HAVE_BUILTIN_CLZ], [], [Whether the compiler supports __builtin_clz])
> +fi
> +
> +AC_MSG_RESULT($support_for_builtin_clz)
> +
>  dnl ==================
>  dnl libpng
>  
> diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
> index 89b9682..4032c13 100644
> --- a/pixman/pixman-matrix.c
> +++ b/pixman/pixman-matrix.c
> @@ -37,7 +37,7 @@
>  static force_inline int
>  count_leading_zeros (uint32_t x)
>  {
> -#ifdef __GNUC__
> +#ifdef HAVE_BUILTIN_CLZ
>      return __builtin_clz (x);
>  #else
>      int n = 0;

Thanks, looks good to me. Please send it using git send-email with
a descriptive commit message and, I think, we can apply it.

Yet another bonus of this solution is that the people trying to
port pixman to different platforms/compilers will have more
chances to see what functionality is useful for pixman by just
looking at the configure output. And, for example, the person
maintaining the MSVC port can optionally provide an efficient CLZ
implementation using MSVC intrinsics. Without the configure check,
this optimization opportunity is buried deeply inside of the
pixman sources and is hard to spot.

And in the unlikely case if some compiler provides an implementation
of __builtin_clz, which does not work in the same way as in GCC (this
would be really strange though), we still have the test suite
("matrix-test" for this particular feature), which should detect
the problem.

BTW, if you have not done this already, you may also want to try
running pixman "make check" with your legacy GCC 3.3 compiler just
to be sure that everything is fine. The bugs in the compilers (or
even in the hardware!) are not so uncommon and pixman test suite
has already found a few of them.

-- 
Best regards,
Siarhei Siamashka