gfxAlphaRecovery.cpp should not be compiled with -msse2

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Blocks: 587936
No longer blocks: 585708
Depends on: 585708
Depends on: 585818
Posted patch Patch v1 (obsolete) — Splinter Review
Requires the patches from bug 585708 and bug 585818.
Comment on attachment 495336 [details] [diff] [review]
Patch v1

Vlad, does this change look OK to you?
Attachment #495336 - Flags: review?(vladimir)
Attachment #495336 - Attachment is obsolete: true
Attachment #495336 - Flags: review?(vladimir)
Posted patch Patch v2Splinter Review
Fixing sunpro line in Makefile, hopefully.
Attachment #495339 - Flags: review?(vladimir)
Comment on attachment 495339 [details] [diff] [review]
Patch v2

looks ok to me; hopefully we can drop non-SSE2 in the near future!
Attachment #495339 - Flags: review?(vladimir) → review+
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.
Attachment #495339 - Flags: approval2.0?
This chain of patches passes try?
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.
blocking2.0: --- → final+
Assignee: nobody → justin.lebar+bug
Attachment #495339 - Flags: approval2.0?
(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.
blocking2.0: final+ → ---
OK, well, in that case - have you run it on try so I can approve it? :)
Just pushed.  :)
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
My patch queue now passes on try.  Requesting approval again.
Attachment #495339 - Flags: approval2.0?
Attachment #495339 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/ae853abfbc86
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
(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?
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.
You need to log in before you can comment on or make changes to this bug.