817058 - AltiVec acceleration for nsTextFragment

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Tobias Netzel]

Attachment: VMX acceleration for nsTextFragment

This implements VMX acceleration for FirstNon8Bit(), inspired by the SSE2 implementation. It depends on runtime VMX detection.

Comment 1

[Tobias Netzel]

Attachment: VMX acceleration for nsTextFragment v2

replaced C cast with C++ cast

Comment 2

[Tobias Netzel]

Attachment: VMX acceleration for nsTextFragment v3

One more style guide issue.

Comment 3

[Tobias Netzel]

Attachment: VMX acceleration for nsTextFragment v4

More style guide issues and a missing cast - sorry for the spam.

Comment 4

[Cameron Kaiser [:spectre]]

bz would probably be an appropriate reviewer for this.

Comment 5

[Tobias Netzel]

Attachment: AltiVec acceleration for nsTextFragment v5

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698261 [details] [diff] [review]
AltiVec acceleration for nsTextFragment v5

Still reading the implementation details, but should there be an AltiVec.h in this diff?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698261 [details] [diff] [review]
AltiVec acceleration for nsTextFragment v5

numUnicharsPerVector should probably be uint32_t.

>+  register vector unsigned short vect;

Why not put this inside the if branch where it's used?

>+  if ((len - alignLen) > (numUnicharsPerVector - 1)) {

Wouldn't >= numUnicharsPerVector be clearer?

>+        CheckForASCII
>+        CheckForASCII

Is that just a manual loop unroll as opposed to something needed for correctness?  If so, worth documenting it as such.

r=me with those nits.

Comment 8

[Tobias Netzel]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 698261 [details] [diff] [review]
> AltiVec acceleration for nsTextFragment v5
> 
> Still reading the implementation details, but should there be an AltiVec.h
> in this diff?

AltiVec.h is part of bug 817042.

Comment 9

[Tobias Netzel]

Attachment: AltiVec acceleration for nsTextFragment v6

The datatypes were signed because in the original SSE version they are signed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698831 [details] [diff] [review]
AltiVec acceleration for nsTextFragment v6

> AltiVec.h is part of bug 817042.

Ah, great, thanks.

> The datatypes were signed because in the original SSE version they are signed.

numUnicharsPerVector is unsigned in the SSE version, as far as I can tell.

r=me

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 698831 [details] [diff] [review]
AltiVec acceleration for nsTextFragment v6

r=jst too fwiw.

Comment 12

[Cameron Kaiser [:spectre]]

Since Tobias is out travelling, requesting checkin on his behalf.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/66483dfa678f

https://tbpl.mozilla.org/php/getParsedLog.php?id=20565230&tree=Mozilla-Inbound
/builds/slave/m-in-osx64-d-00000000000000000/build/content/base/src/nsTextFragment.cpp:20:10: fatal error: 'mozilla/AltiVec.h' file not found

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, see the "depends on" field....

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Yeah, see the "depends on" field....

With due respect, the expectation is that checkin-needed is being set when the patch (and any dependencies) are ready for checkin. I didn't decide to just land this on a whim.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yes, I know.  I wasn't blaming you; the checkin-needed was clearly set incorrectly.  Sorry I wasn't clear about that.

I just wonder whether there's any way to automatically catch that sort of thing in queries to reduce the load on people pushing checkin-needed stuff.  e.g. having a separate query for things that are checkin-needed but have open "depends on" bugs, where you'd want to double-check before pushing.  Or something.  Not sure what a sane setup for that is.

Comment 17

[Cameron Kaiser [:spectre]]

That was my fault. Apologies.

Comment 18

[Cameron Kaiser [:spectre]]

Now that bug 1571613 has landed, I'm dusting this one off and updating it (for the new build system and to support big and little endian).

Comment 19

[Cameron Kaiser [:spectre]]

Attachment: Bug 817058 - VMX acceleration for nsTextFragment. r?bzbarsky

Comment 20

[Cameron Kaiser [:spectre]]

Attachment: Bug 817058 - VMX acceleration for nsTextFragment (v2). r?bzbarsky

Comment 21

[Cameron Kaiser [:spectre]]

Attachment: Bug 817058 - VMX acceleration for nsTextFragment. r?bzbarsky

Comment 22

[Cameron Kaiser [:spectre]]

I apologize, but I can't get moz-phab to stop making new differential revisions for my revised patches, which is why I opened a new one and abandoned the previous one.

Comment 23

[Cameron Kaiser [:spectre]]

Thanks for the review and sorry for the churn.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3fe24ea6309
VMX acceleration for nsTextFragment. r=bzbarsky

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d3fe24ea6309

Comment 26

[Henri Sivonen (:hsivonen)]

It depends on runtime VMX detection.

I'm planning to migrate TextFragment to use encoding_rs::mem. It currently supports SIMD acceleration for our tier-1 architectures: x86+SSE2, x86_64, armv7+NEON, and aarch64. In all these cases, the SIMD support is present unconditionally. It could support VMX but I'd like that to be unconditional, too, because it's rather important for the SIMD code to get inlined without function call overhead.

Is the run-time detection that's in the patch still relevant? That is, don't POWER8 and POWER9 have VMX unconditionally?

Comment 27

[Cameron Kaiser [:spectre]]

POWER6 and up have VMX unconditionally, as does the POWER4-derived G5, but not all 64-bit Power CPUs do (the most notorious example is probably the QorIQ P5020 as used in the AmigaOne X5000, which runs Linux and AmigaOS). I agree I'd prefer that too but I can't guarantee that for ppc64 even though I'm not a fan of the choices the Amiga community is making with their designs.

That said, if the demand is that all tiers of Firefox should be SIMD-capable, that's not a policy question I can answer (though it won't affect the POWER9 I'm typing this on).

Comment 28

[Henri Sivonen (:hsivonen)]

POWER6 and up have VMX unconditionally, as does the POWER4-derived G5, but not all 64-bit Power CPUs do (the most notorious example is probably the QorIQ P5020 as used in the AmigaOne X5000, which runs Linux and AmigaOS).

Does Firefox run on AmigaOne? It's big-endian, right? Does Firefox still run on big-endian systems generally? I thought Skia dropped support for big-endian architectures, and asm.js/Wasm require little-endian ArrayBuffer semantics. Also, it seems that on 32-bit PPC Ubuntu, the Firefox package stopped updating while other 16.04 packages were still getting security updates. (I haven't booted up my PPC Ubuntu install in a while, so I don't know what the current situation is.)

POWER6 and up have VMX unconditionally

OK, so ppc64le has VMX unconditionally, right? (ppc64le is POWER 8 and up, right?)

Comment 29

[Cameron Kaiser [:spectre]]

I don't know about AmigaOne, but there are still big-endian ppc64 builds of mainline Firefox and to the best of my knowledge, these do operate. Adelie Linux is probably the best known. The big-endian builds generally don't support asm.js (TenFourFox got around this by simulating little-endianness for typed arrays).

To the best of my knowledge, all ppc64le-capable CPUs support VMX (correct, POWER8 and up).