Use NEON build flag on ARM and AArch64 #10916

mmatyas · 2016-04-29T10:25:52Z

The NEON flag is already used when building for Android, this patch enables it on other ARM devices too.

Note that this patch just adds the build flag to the compilation, for actually enabling the SIMD code in Servo, we'll also need #10900 (but it's not a dependency).

This change is

highfive · 2016-04-29T10:25:55Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/build_commands.py

wafflespeanut · 2016-04-29T11:33:23Z

python/servo/build_commands.py

@@ -202,9 +202,13 @@ def build(self, target=None, release=False, dev=False, jobs=None,

        build_start = time()
        env = self.build_env()
+
+        # Ensure Rust uses hard floats and SIMD on ARM devices
+        if len(targets):


This could just be targets :)

aneeshusa · 2016-04-29T16:00:29Z

python/servo/build_commands.py

@@ -202,9 +202,13 @@ def build(self, target=None, release=False, dev=False, jobs=None,

        build_start = time()
        env = self.build_env()
+
+        # Ensure Rust uses hard floats and SIMD on ARM devices
+        if targets:


Personally, I prefer the more explicit check of if len(targets) > 0 here.

mmatyas · 2016-04-29T16:00:41Z

Ah, right, thanks :)

aneeshusa · 2016-04-29T16:02:08Z

Only looking at targets[0] seems strange to me. Do we only support one target at a time? If so, why is targets a list? If not, I would think (from a very cursory glance) that we'd need to examine the other targets as well.

mmatyas · 2016-04-29T16:13:21Z

targets being a list is a good catch, it doesn't look like we add more than one element to it. I think technically it would be possible to build multiple targets, but right now only one is expected or Android.

About the if, would you prefer len(targets) > 0 then?

aneeshusa · 2016-04-29T16:49:24Z

It seems the underlying cargo build command here does not accept multiple --target specifications, so targets should not be a list here. (We should also update ensure_bootstrapped to take a single target instead of a list.)

larsbergstrom · 2016-04-29T16:51:51Z

Hrm, I thought that targets was a list because you always need the host bits (because of compiling compiler plugins, which run on the host) plus you will need a second one if you have a cross-compile.

aneeshusa · 2016-04-29T16:55:33Z

I don't see any places where the host target is added to the targets list; it's just the one cross-compile target (e.g. ["arm-linux-androideabi"] or an explicit target). It would make sense that you need the host target to actually compile, but I don't see it being used in the targets variable.

larsbergstrom · 2016-04-29T17:07:05Z

yeah, that all happens here:
https://github.com/servo/servo/blob/master/python/servo/bootstrap_commands.py#L168-L169

aneeshusa · 2016-04-29T17:17:09Z

Convo on IRC: http://logs.glob.uno/?c=mozilla%23servo#c418555

Let's make the targets variables in build_commands.py not lists, and do the same for the targets argument to ensure_bootstrapped in command_base.py since it's only called with that argument from build_commands.py. (The bootstrap_rustc function in bootstrap_commands.py handles inclusion of the host triple.)

mmatyas · 2016-04-29T17:28:15Z

Ok! And what should happen if both a target and --android is defined? Should --android override --target?

aneeshusa · 2016-04-29T17:56:10Z

@mmatyas Let's make them mutually exclusive, like --release and --dev are. We can only build for one target at a time.

aneeshusa · 2016-04-29T18:07:18Z

Also, bonus points if you make a separate commit for making targets non-lists first, and then a separate commit after that for the NEON build flag change.

mmatyas · 2016-04-29T18:26:36Z

Ok, here's an update. My Python might be a little rusty, so feel free to point on any mistakes.

Just two things you might notice:
I've also changed the if android branch; now it sets the target value from self.config["android"]["target"]], (which is by default arm-linux-androideabi) instead of hardcoding. If, for some reason you use a different Android target, you'd probably need the matching stdlibs, so hardcoding to arm-linux-androideabi and bootstrapping that might be useless.
And, Registrar.dispatch("bootstrap-rust", context=self.context, target=[target]) gets the target as [target], to match the expected argument format.

It you think this looks good, I'll make the separate commits ( ❤️ challenges).

aneeshusa · 2016-04-29T19:26:02Z

python/servo/command_base.py

@@ -411,7 +411,7 @@ def android_support_dir(self):
    def android_build_dir(self, dev):
        return path.join(self.get_target_dir(), "arm-linux-androideabi", "debug" if dev else "release")

-    def ensure_bootstrapped(self, targets=[]):
+    def ensure_bootstrapped(self, target=""):


The default here should be target=None, not target="". We only need to do the target-related things if we are passed a target explicitly.

aneeshusa · 2016-04-29T19:30:56Z

These changes look good aside from the default argument there. I didn't realize you could fixup fixup commits - I'm surprised the last one isn't fixup! fixup! fixup! fixup! 😛 Feel free to go ahead and reorder these commits.

wafflespeanut · 2016-04-29T19:35:59Z

This should actually be squashed...

mmatyas · 2016-04-29T20:53:12Z

Rebased and grouped to two commits.

(PS. Sorry for the late replies)

mmatyas · 2016-04-29T21:42:07Z

Is there a clean, Pythonic way to remove None from a list before passing as a function argument?

wafflespeanut · 2016-04-29T21:56:01Z

filter probably?

filter(lambda i: i, [True, None, 'foo'])

wafflespeanut · 2016-04-29T21:57:21Z

btw, it would be nice if we don't have "fixup!" in a commit message :)

aneeshusa · 2016-04-29T21:59:57Z

Even better, we can do filter(None, some_list). See https://docs.python.org/2/library/functions.html#filter - passing None is special cased to the identity function.

@wafflespeanut git can automatically combine fixup! commits with rebase --autosquash: https://git-scm.com/docs/git-rebase, so this makes reviewing easier.

bors-servo · 2016-05-05T19:17:38Z

⌛ Testing commit 0bcf35c with merge f3faf08...


        Auto merge of #10916 - mmatyas:useneon, r=aneeshusa

Use NEON build flag on ARM and AArch64 The NEON flag is already used when building for Android, this patch enables it on other ARM devices too. Note that this patch just adds the build flag to the compilation, for actually enabling the SIMD code in Servo, we'll also need #10900 (but it's not a dependency).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10916)

bors-servo · 2016-05-05T19:23:32Z

💔 Test failed - windows

mmatyas · 2016-05-05T19:37:01Z

[2/-1] Failed to receive response: The server returned an invalid or unrecognized response

Looks like another network issue.

larsbergstrom · 2016-05-05T19:44:53Z

@bors-servo retry

infra

bors-servo · 2016-05-05T20:49:32Z

⌛ Testing commit 0bcf35c with merge 9c9c6ea...


        Auto merge of #10916 - mmatyas:useneon, r=aneeshusa

Use NEON build flag on ARM and AArch64 The NEON flag is already used when building for Android, this patch enables it on other ARM devices too. Note that this patch just adds the build flag to the compilation, for actually enabling the SIMD code in Servo, we'll also need #10900 (but it's not a dependency).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10916)

bors-servo · 2016-05-05T20:56:48Z

💔 Test failed - windows

jdm · 2016-05-05T21:00:43Z

@bors-servo: retry

infra

bors-servo · 2016-05-05T21:44:04Z

⌛ Testing commit 0bcf35c with merge a233d1e...


        Auto merge of #10916 - mmatyas:useneon, r=aneeshusa

Use NEON build flag on ARM and AArch64 The NEON flag is already used when building for Android, this patch enables it on other ARM devices too. Note that this patch just adds the build flag to the compilation, for actually enabling the SIMD code in Servo, we'll also need #10900 (but it's not a dependency).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10916)

bors-servo · 2016-05-05T22:31:29Z

💔 Test failed - linux-rel

highfive · 2016-05-05T22:31:31Z

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, placeholderWidthAttr: &#39;100&#39;, placeholderHeightAttr: &#39;100%&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Test timed out

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-auto.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-auto.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, containerHeightStyle: &#39;400px&#39;, placeholderWidthAttr: &#39;50%&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Test timed out

jdm · 2016-05-05T22:58:53Z

@bors-servo: retry

#9909 #10457

bors-servo · 2016-05-05T23:26:49Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

bors-servo · 2016-05-06T00:13:53Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned nox Apr 29, 2016

highfive added the S-awaiting-review label Apr 29, 2016

mmatyas mentioned this pull request Apr 29, 2016

Use SIMD in `gfx` when possible #10900

Closed

wafflespeanut reviewed Apr 29, 2016
View changes

aneeshusa reviewed Apr 29, 2016
View changes

nox assigned aneeshusa and unassigned nox Apr 29, 2016

aneeshusa reviewed Apr 29, 2016
View changes

wafflespeanut added the S-needs-squash label Apr 29, 2016

mmatyas force-pushed the mmatyas:useneon branch from adec454 to 07a23a0 Apr 29, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 5, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 5, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 5, 2016

highfive removed the S-awaiting-merge label May 5, 2016

highfive added the S-tests-failed label May 5, 2016

bors-servo merged commit 0bcf35c into servo:master May 6, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 6, 2016

servo / servo

Use NEON build flag on ARM and AArch64 #10916

Use NEON build flag on ARM and AArch64 #10916

mmatyas commented Apr 29, 2016 •

edited by larsbergstrom

highfive commented Apr 29, 2016

This comment has been minimized.

This comment has been minimized.

mmatyas commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

mmatyas commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

larsbergstrom commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

larsbergstrom commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

mmatyas commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

mmatyas commented Apr 29, 2016

This comment has been minimized.

aneeshusa commented Apr 29, 2016

wafflespeanut commented Apr 29, 2016

mmatyas commented Apr 29, 2016

mmatyas commented Apr 29, 2016 •

edited

wafflespeanut commented Apr 29, 2016

wafflespeanut commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

bors-servo commented May 5, 2016

bors-servo commented May 5, 2016

mmatyas commented May 5, 2016

larsbergstrom commented May 5, 2016

bors-servo commented May 5, 2016

bors-servo commented May 5, 2016

jdm commented May 5, 2016

bors-servo commented May 5, 2016

bors-servo commented May 5, 2016

highfive commented May 5, 2016

jdm commented May 5, 2016 •

edited

bors-servo commented May 5, 2016

bors-servo commented May 6, 2016

servo / servo

Join GitHub today

Use NEON build flag on ARM and AArch64 #10916

Use NEON build flag on ARM and AArch64 #10916

Conversation

mmatyas commented Apr 29, 2016 • edited by larsbergstrom

highfive commented Apr 29, 2016

This comment has been minimized.

wafflespeanut Apr 29, 2016

This comment has been minimized.

aneeshusa Apr 29, 2016

mmatyas commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

mmatyas commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

larsbergstrom commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

larsbergstrom commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

mmatyas commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

mmatyas commented Apr 29, 2016

This comment has been minimized.

aneeshusa Apr 29, 2016

aneeshusa commented Apr 29, 2016

wafflespeanut commented Apr 29, 2016

mmatyas commented Apr 29, 2016

mmatyas commented Apr 29, 2016 • edited

wafflespeanut commented Apr 29, 2016

wafflespeanut commented Apr 29, 2016

aneeshusa commented Apr 29, 2016

bors-servo commented May 5, 2016

bors-servo commented May 5, 2016

mmatyas commented May 5, 2016

larsbergstrom commented May 5, 2016

bors-servo commented May 5, 2016

bors-servo commented May 5, 2016

jdm commented May 5, 2016

bors-servo commented May 5, 2016

bors-servo commented May 5, 2016

highfive commented May 5, 2016

jdm commented May 5, 2016 • edited

bors-servo commented May 5, 2016

bors-servo commented May 6, 2016

mmatyas commented Apr 29, 2016 •

edited by larsbergstrom

mmatyas commented Apr 29, 2016 •

edited

jdm commented May 5, 2016 •

edited