Disable SSE2

Disable SSE2 build option on Chromium.
BUG=8475, 39969, 28981
TEST=Chromium should build and run, including layout tests, on a Pentium2 or later.

[fbarchard, 2010-04-20 01:55:20]

Use fpu sse for webkit to pass layout tests consistently, but do not enable
msse2 which prevents Athlon and older Pentium2/3 from working.

[agl, 2010-04-20 02:16:40]

NACK until I get around to this in the morning.

On Apr 19, 2010 9:55 PM, <fbarchard@chromium.org> wrote:

Reviewers: agl,

Message:
Use fpu sse for webkit to pass layout tests consistently, but do not enable
msse2 which prevents Athlon and older Pentium2/3 from working.

Description:
Disable SSE2 by default.
A new GYP_DEFINES=enable_sse2=1 is also exposed, but off by default, allow a
user to build chromium with sse2.
BUG=8475
TEST=Chromium should build and run, including layout tests, on a Pentium2 or
later.


Please review this at http://codereview.chromium.org/1611034/show

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
 M     build/common.gypi


Index: build/common.gypi
===================================================================
--- build/common.gypi   (revision 44998)
+++ build/common.gypi   (working copy)
@@ -83,6 +83,9 @@
      # libraries on linux x86-64 and arm.
      'linux_fpic%': 0,

+      # Set to 1 to build with -msse2.
+       'enable_sse2%': 0,
+
      # Python version.
      'python_ver%': '2.5',

@@ -107,6 +110,7 @@
    'inside_chromium_build%': '<(inside_chromium_build)',
    'fastbuild%': '<(fastbuild)',
    'linux_fpic%': '<(linux_fpic)',
+    'enable_sse2%': '<(enable_sse2)',
    'python_ver%': '<(python_ver)',
    'armv7%': '<(armv7)',
    'arm_neon%': '<(arm_neon)',
@@ -231,7 +235,7 @@

    # Enable new NPDevice API.
    'enable_new_npdevice_api%': 0,
-
+
    'conditions': [
      ['OS=="linux" or OS=="freebsd" or OS=="openbsd"', {
        # This will set gcc_version to XY if you are running gcc X.Y.*.
@@ -882,7 +886,7 @@
            # to its original precision) but they have significant runtime
            # performance penalty.
            #
-            # -mfpmath=sse -msse2 makes the compiler use SSE instructions
+            # -mfpmath=sse -msse makes the compiler use SSE instructions
            # which keep floating-point values in SSE registers in its
            # native precision (32-bit for single precision, and 64-bit for
            # double precision values). This means the floating-point value
@@ -892,8 +896,7 @@
            'conditions': [
              ['branding=="Chromium"', {
                'cflags': [
-                  '-march=pentium4',
-                  '-msse2',
+                  '-msse',
                  '-mfpmath=sse',
                ],
              }],
@@ -901,7 +904,7 @@
              # benefit comes from sse2 so this setting allows ChromeOS
              # to build on other CPUs.  In the future -march=atom would
help
              # but requires a newer compiler.
-              ['chromeos==1', {
+              ['chromeos==1 or enable_sse2==1', {
                'cflags': [
                  '-msse2',
                ],

[fbarchard, 2010-04-20 06:38:38]

Note the purpose of this change is to allow chromium to build/run on non-sse2
machines - namingly Athlon and Pentium2.
I've kept the change to that.  But as followup, I think we should add sse2 and
march=core to ChromeOS

http://codereview.chromium.org/1611034/diff/10001/11001
File build/common.gypi (left):

http://codereview.chromium.org/1611034/diff/10001/11001#oldcode895
build/common.gypi:895: '-march=pentium4',
remove pentium4, because users want chromium to run on pentium2 and athlon

http://codereview.chromium.org/1611034/diff/10001/11001#oldcode896
build/common.gypi:896: '-msse2',
reduce from sse2 to sse, which is sufficient for float math and works on
pentium2

[agl, 2010-04-20 14:01:32]

NACK.

The intention is that Chrome builds don't include these flags (because some
people have really old systems), but that Chromium (dev) builds *do*.

Mostly this is to keep the floating point math sane and stop huge numbers of
layout tests from flaking. Also, it's pretty much impossible to build Chromium
on anything older than a P4.

Distributors can tweak the settings to match their distro's policies, but that's
out of our hands.

So I don't see the need for this change. Even if it were to land, you would need
to check the layout tests to make sure that it doesn't cause small differences
in the pixel test results.

[scherkus (not reviewing), 2010-04-20 23:17:57]

Adding fta (Ubuntu PPA maintainer) for additional input.

[fbarchard, 2010-04-20 23:37:52]

This change only affects Chromium; No change to Chrome/ChromeOS.
-mfpmath=sse is still the same, and the key to getting Webkit layout tests to
pass.

I'm suggesting we replace -msse2 with -msse.  This should have no impact on the
float code.

Removing -march=pentium4 should not impact Webkit quality.

By putting it behind a flag, we can still enable_sse2, including for Chrome, if
there is a reason to.

[tony, 2010-04-21 01:26:04]

On 2010/04/20 23:37:52, fbarchard wrote:
> This change only affects Chromium; No change to Chrome/ChromeOS.
> -mfpmath=sse is still the same, and the key to getting Webkit layout tests to
> pass.

Have you verified that the layout tests pass with this change?  Both debug and
release builds?

[fbarchard, 2010-04-21 03:35:36]

I did this, and it passed
gcl try DisableSSE2 -S layout_linux

How do I run those tests myself?  What I tried is
build 32 bit chromium test-shell release mode
cd src/third_party/WebKit/WebKitTools/Scripts
./new-run-webkit-tests
that kinda worked, but without the patch applied, it failed many test, so I'm
not building/running the tests properly.

layout tests should pass, because we're still using SSE for floats.

[tony, 2010-04-21 03:57:59]

On 2010/04/21 03:35:36, fbarchard wrote:
> I did this, and it passed
> gcl try DisableSSE2 -S layout_linux

You want -b layout_linux.  I would expect to see a box in reitveld labeled
layout_linux if it had run on the try bots.  This would test a debug build.

> How do I run those tests myself?  What I tried is
> build 32 bit chromium test-shell release mode
> cd src/third_party/WebKit/WebKitTools/Scripts
> ./new-run-webkit-tests
> that kinda worked, but without the patch applied, it failed many test, so I'm
> not building/running the tests properly.
> 
> layout tests should pass, because we're still using SSE for floats.

http://code.google.com/p/chromium/wiki/LinuxBuildInstructions (see the Release
mode section)
http://code.google.com/p/chromium/wiki/LinuxBuild32On64 (the bottom has the gyp
variable you have to set before running gclient runhooks)
http://www.chromium.org/developers/testing/webkit-layout-tests

[fta, 2010-04-21 06:16:50]

On 2010/04/20 23:17:57, scherkus wrote:
> Adding fta (Ubuntu PPA maintainer) for additional input.

lgtm, thanks

[fbarchard, 2010-04-22 02:54:27]

There seems to be a difference on the svg layout tests?
Could we rebase for that?

[tony, 2010-04-22 02:58:30]

On 2010/04/22 02:54:27, fbarchard wrote:
> There seems to be a difference on the svg layout tests?
> Could we rebase for that?

I believe that is the original problem.  Debug and Release expect different
results.  We don't have separate results for Debug vs Release.

[fbarchard, 2010-04-22 04:31:03]

This shouldnt be different for debug vs release, because its using SSE for
float.

[tony, 2010-04-22 04:43:19]

On 2010/04/22 04:31:03, fbarchard wrote:
> This shouldnt be different for debug vs release, because its using SSE for
> float.

It's the usage of "shouldn't" that makes me nervous about this change. :)

To clarify, you're saying that Debug and Release on your machine failed on the
same tests and they failed in the same way?  That is, if you run the tests on
your machine with --new-baseline in Debug, then you re-run the tests in Release,
Release passes.

fta: If this is causing problems for you in the ubuntu package, you should just
patch it out (like we do for google_chrome builds).

[fbarchard, 2010-04-24 01:27:48]

14 tests need rebasing:

svg/W3C-SVG-1.1/animate-elem-24-t.svg
svg/W3C-SVG-1.1/animate-elem-80-t.svg
svg/W3C-SVG-1.1/filters-comptran-01-b.svg
svg/W3C-SVG-1.1/render-elems-06-t.svg
svg/W3C-SVG-1.1/render-elems-07-t.svg
svg/W3C-SVG-1.1/render-elems-08-t.svg
svg/W3C-SVG-1.1/render-groups-01-b.svg
svg/W3C-SVG-1.1/render-groups-03-t.svg
svg/css/circle-in-mask-with-shadow.svg
svg/custom/feComponentTransfer-Table.svg
svg/custom/glyph-setting-d-attribute.svg
svg/custom/scrolling-embedded-svg-file-image-repaint-problem.html
svg/custom/svg-fonts-in-html.html
svg/overflow/overflow-on-inner-svg-element.svg

3 tests dont match debug vs release

svg/W3C-SVG-1.1/filters-comptran-01-b.svg
  Image mismatch expected actual diff

svg/css/circle-in-mask-with-shadow.svg
  Image mismatch expected actual diff

svg/custom/feComponentTransfer-Table.svg
  Image mismatch expected actual diff

would it be acceptable to disable those 3 tests and move ahead with the change?

[tony, 2010-04-26 00:47:23]

On 2010/04/24 01:27:48, fbarchard wrote:
> would it be acceptable to disable those 3 tests and move ahead with the
change?

I don't think so because these tests would then never pass.  Also, new tests
could fail in the same way and it'll be very hard to identify this is the cause
(it would add a lot of pain to webkit gardeners).

If you can find a set of flags that produce consistent results in Debug and
Release, then we can use those, but I'm not sure why you want this change in the
first place.

[tony, 2010-04-26 00:48:48]

BTW, I think adding a flag to make things easier for FTA is a good idea, but
that doesn't mean we have to change the default set of flags for Chromium
builds.

[fbarchard, 2010-04-26 17:33:22]

The main reason for this change is users want Chromium to run on machines
without mmx2.
Which is why fta patches our builds.  We could make it easier for everyone to
patch the builds, but I think it makes more sense to make Chromium build with
the options users want.
Secondary - for testing, it would help Chrome if Chromium tested with the same
build options - often Chromium will work, and Chrome fails.

On ffmpeg we had a problem that the code wont build with -O0, so we added the
following:
      'debug_optimize': '2',
      'mac_debug_optimization': '2',
It affects the ability to debug, but makes the code work consistently.

[tony, 2010-04-27 00:21:56]

On 2010/04/26 17:33:22, fbarchard wrote:
> The main reason for this change is users want Chromium to run on machines
> without mmx2.
> Which is why fta patches our builds.  We could make it easier for everyone to
> patch the builds, but I think it makes more sense to make Chromium build with
> the options users want.

This is orthogonal.  We can add a flag w/o having to change the default Chromium
build flags.  I will do this so we can help fta sooner.

> Secondary - for testing, it would help Chrome if Chromium tested with the same
> build options - often Chromium will work, and Chrome fails.

I agree with this in principle, but it neglects the consequence that we lose
testing coverage and consistency.  That is, I think the cost to testing is worse
than the gain.

[fbarchard, 2010-04-27 01:40:12]

So we're in agreement on the flag.  If specified, enable_sse2 is respected. 0 is
off.  1 is on.  If unspecified, it has a default value.

But I would like to see enable_sse2 off by default on both chrome and chromium. 
This would improve test coverage/reduce cost.

[agl, 2010-04-27 14:05:18]

On Mon, Apr 26, 2010 at 9:40 PM,  <fbarchard@chromium.org> wrote:
> So we're in agreement on the flag.  If specified, enable_sse2 is respected.
> 0 is
> off.  1 is on.  If unspecified, it has a default value.
>
> But I would like to see enable_sse2 off by default on both chrome and
> chromium.
> This would improve test coverage/reduce cost.

We're either having a flag which is on by default (saving more
rebaselining), or we're just removing SSE2 and not having a flag

Having a flag which is off by default is not ok unless it's part of a
transition towards removing the flag.


AGL

[fbarchard, 2010-05-17 22:46:35]

Would you be okay with unconditionally turning off SSE2?
Layout tests would need to be rebaselined.

[tony, 2010-05-18 00:34:54]

On 2010/05/17 22:46:35, fbarchard wrote:
> Would you be okay with unconditionally turning off SSE2?
> Layout tests would need to be rebaselined.

fbarcherd and I talked offline.  He's going to try to find and fix the webkit
code that is responsible for causing the differences in debug vs release.  after
that, he'll remove the sse2 flag.

[Evan Martin, 2010-05-18 12:05:00]

I wish you luck!

On Tue, May 18, 2010 at 1:34 AM,  <tony@chromium.org> wrote:
> On 2010/05/17 22:46:35, fbarchard wrote:
>>
>> Would you be okay with unconditionally turning off SSE2?
>> Layout tests would need to be rebaselined.
>
> fbarcherd and I talked offline.  He's going to try to find and fix the
> webkit
> code that is responsible for causing the differences in debug vs release.
>  after
> that, he'll remove the sse2 flag.
>
> http://codereview.chromium.org/1611034/show
>