461202 - Turn on SSE2 pixman optimizations

Status: VERIFIED FIXED

(Core :: Graphics, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Right now, we still have the SSE2 optimizations disabled in pixman for various reasons on various platforms.

For linux, we need bug 410509 (or similar) fixed; otherwise builds crash.

On win32, where that problem doesn't exist, I was seeing reftest failures the last time I tried this.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Shouldn't the __attribute__((__force_align_arg_pointer__)) in pixman deal with this? Or do we want to enable this for compilers that don't support __force_align_arg_pointer?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It should, but it didn't seem to -- it's entirely possible that the compiler I was trying with (what's on the tinderbox -- centos 5.1, maybe 5.2) doesn't support it.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

It looks like we currently build with gcc 4.1 so we don't have
__force_align_arg_pointer. I don't know if bug 410509 is sufficient to
guarantee 128bit alignment for all the pixman calls...

Comment 4

[Nomis101]

While Bug 410509 is now fixed, is it now possible to turn on SSE2 in pixman without any risk? Or is there anything else to do before?

Comment 5

[Mark Perris]

The reftests will almost certainly need updating, as the SSE2 code paths produce slighly different results for color operations, etc, due to rounding differences.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

The reftests seem to pass for me. There should be no different results for color operations as the sse2 paths should produce identical results.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Enable SSE2 on platforms that support it.

This includes MSVC and GCC's with __force_align_arg_pointer__.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Awesome, toss this on the try server?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I did some time ago. I don't remember there being any interesting results. I've started another run though.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, ok -- just making sure that talos succeeded really.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

The reftests now seem to be failing...

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Nevermind, I was testing a broken build. I don't see any reason not to land this.

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b98316b61797

Comment 14

[Dão Gottwald [:dao]]

Backed out because of leak test failure on Linux.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Screw it; linux doesn't get to come along for this party.  It's irrelevant anyway, we won't really be using our pixman on linux for much.  Was win32 green and stuff?

Comment 16

[Dão Gottwald [:dao]]

Yep, Windows was all green.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I don't see how this patch could have caused a leak test failure as it shouldn't have caused any additional allocations, so either the it was false failure or it's pointing some other badness that this patch causes.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I don't think it caused leaks, it looks like it caused that box to go orange, which often happens when things crash.  So I'm guessing we're running into the SSE2 stack alignment issue, and the xpcom patch and the function alignment attribute isn't enough to fix it.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: Enable SSE2 on MSVC [Checkin: Comment 20]

Only enable SSE2 on MSVC for now.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 349782 [details] [diff] [review]
Enable SSE2 on MSVC
[Checkin: Comment 20]

http://hg.mozilla.org/mozilla-central/rev/55b681f2d821

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> (From update of attachment 349782 [details] [diff] [review])
> http://hg.mozilla.org/mozilla-central/rev/55b681f2d821

Note that there was
{
patching file gfx/cairo/libpixman/src/Makefile.in
Hunk #1 succeeded at 69 with fuzz 2 (offset 2 lines).
}
In this case, you may want to check if the result is as wanted.!.

Comment 22

[Nomis101]

Maybe Bug 447982 is of interest here.

Comment 23

[Mark Perris]

I wonder if this is responsible for the big DHTML perf regression on Vista?
It shouldn't be, but the times are close.

http://graphs.mozilla.org/graph.html#show=1431894,787166,787165,787152

Comment 24

[Jeff Muizelaar [:jrmuizel]]

It could be. I'm surprised that any performance regressions wouldn't also show up on XP though.

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/77ee5d37fdf5

Comment 26

[Nomis101]

Does this make any problems on Mac OS X because you only turned it on for windows? Or aren't there any advantages for Intel-Macs?

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No real advantages on mac; we don't use pixman much there.  Only mild advantages on linux, and it causes problems due to gcc, so it's not enabled there either.