616782 - gfxAlphaRecovery.cpp should not be compiled with -msse2

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Justin Lebar (not reading bugmail)]

Split off from bug 587936 comment 14.

In gfx/thebes/Makefile.in:

> ifeq (86,$(findstring 86,$(OS_TEST)))
> ifdef __GNUC__
> gfxAlphaRecovery.$(OBJ_SUFFIX): MODULE_OPTIMIZE_FLAGS += -msse2
> endif

This is rather dangerous.  Setting -msse2 gives GCC permission to use sse2
instructions in the file wherever it pleases, even outside your CPUID guard. 
I'm not sure if recent versions of GCC will add SSE instructions at -Os (which
is what we're currently using on Linux), but I know it will at -O3, which is
what we're switching to in the near future.

The only correct way that I'm aware of to use SSE intrinsics with GCC is to
place them in a separate file.

This can be fixed along with bug 585708.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Requires the patches from bug 585708 and bug 585818.

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 495336 [details] [diff] [review]
Patch v1

Vlad, does this change look OK to you?

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Fixing sunpro line in Makefile, hopefully.

Comment 4

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

Comment on attachment 495339 [details] [diff] [review]
Patch v2

looks ok to me; hopefully we can drop non-SSE2 in the near future!

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 495339 [details] [diff] [review]
Patch v2

Requesting approval.  This should be a safe change -- it just moves code around.  The benefits are making this vectorized code available on Linux 32 and being sure that gcc won't add SSE2 instructions into the unvectorized portion of the existing code.

Comment 6

[Joe Drew (not getting mail)]

This chain of patches passes try?

Comment 7

[Joe Drew (not getting mail)]

Also, given that this will actually cause us to fail to run on non-SSE2 processors, this should block final. AFAIK we still haven't started requiring SSE2.

Comment 8

[Justin Lebar (not reading bugmail)]

(In reply to comment #7)
> Also, given that this will actually cause us to fail to run on non-SSE2
> processors, this should block final. AFAIK we still haven't started requiring
> SSE2.

I don't think this will cause a crash on non-SSE2 processors unless GCC decides to vectorize the unvectorized loop in gfxAlphaRecovery.cpp.  Contrast with bug 616778.

Comment 9

[Joe Drew (not getting mail)]

OK, well, in that case - have you run it on try so I can approve it? :)

Comment 10

[Justin Lebar (not reading bugmail)]

Just pushed.  :)

Comment 11

[Justin Lebar (not reading bugmail)]

Well, the good news is that this doesn't break anything on Mac or Linux.  The bad news is that it burns the Windows build.  :)

I'll have a closer look when I can; maybe Friday or the weekend.  It's probably just something silly.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291754828.1291759934.2499.gz

Comment 12

[Justin Lebar (not reading bugmail)]

My patch queue now passes on try.  Requesting approval again.

Comment 13

[Justin Lebar (not reading bugmail)]

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

Comment 14

[[e-mail]]

Comment on attachment 495339 [details] [diff] [review]
Patch v2

I'm not sure how it worked before, but MOZILLA_MAY_SUPPORT_SSE2 appears to be undefined on my system, and gfxAlphaRecoverySS2.cpp thus fails to compile.

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to comment #14)
> Comment on attachment 495339 [details] [diff] [review]
> Patch v2
> 
> I'm not sure how it worked before, but MOZILLA_MAY_SUPPORT_SSE2 appears to be
> undefined on my system, and gfxAlphaRecoverySS2.cpp thus fails to compile.

Could you please give some details about your system and how you're building?  You did a reconfigure, right?

Comment 16

[Justin Lebar (not reading bugmail)]

Neil tells me that the failure above occurred with MSVC 7.1 (VS 2003).  We're explicitly not supporting this version and iirc we knowingly break it elsewhere.  I can dig up the bug where that discussion took place, if anyone is interested.