1722102 - Import intgemm library as third_party/ sources

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Yury Delendik (:yury)]

To implement bug 1720747 intrinsic functions, we want to import https://github.com/kpu/intgemm sources. The current version of the library, that is used in the extension, is 6228d016ecc63470d2dbb76bd4ab7b0abe097993, though it is likely to change when intrinsic interface is identified.

The FF build system requires -fno-exceptions set -- I filed https://github.com/kpu/intgemm/issues/91 for that

Comment 1

[Yury Delendik (:yury)]

Attachment: Bug 1722102 - Adding vendor intgemm library r=yury,lth,glandium

Comment 2

[Abhishek]

Yury. I tested the latest master of intgemm and it works with the gemm interface. Kenneth also recommended the same sources to import in Firefox. The -fno-exceptions change is already in the latest master.

Therefore, we can go ahead with the latest master.

Comment 3

[André Natal]

I'm continuing the work that yury started until we land it.

I've just pushed a new patch to try to determine the failure points in order to find the guards for the specific CPU extensions we'll need to implement/utilize, but I wanted to already start the discussion about it with the reviewers.

remote:
remote: View your changes here:
remote: https://hg.mozilla.org/try/rev/d5472d0f0e99b4e079fc7085e02725fc47a52c53
remote: https://hg.mozilla.org/try/rev/206cad4c6d49009a829a7a1b0c9a22e5cb16dde9
remote:
remote: Follow the progress of your build on Treeherder:
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=206cad4c6d49009a829a7a1b0c9a22e5cb16dde9
remote: recorded changegroup in replication log in 0.008s
To hg::ssh://hg.mozilla.org/try

• [new branch] HEAD -> branches/default/tip

Comment 4

[Jerin Philip]

Suspecting a few failures have something to do with

• https://hg.mozilla.org/try/diff/d5472d0f0e99b4e079fc7085e02725fc47a52c53/js/src/intgemm/detect_avx2.py
• https://hg.mozilla.org/try/diff/d5472d0f0e99b4e079fc7085e02725fc47a52c53/js/src/intgemm/moz.build

#cmakedefine VAR ...
will be replaced with either

#define VAR ...
or

/* #undef VAR */

The variable is supposed to not exist if the hardware supporting it is not detected, hence a few AVX512 build failures.

Comment 5

[Yury Delendik (:yury)]

It makes sense detect and limit everything for AVX2 only (or even to SSSE3). We also want to "filter out" non-x86 platforms for this iteration and avoid any build time for intgemm on such platforms.

Comment 6

[Jerin Philip]

It makes sense detect and limit everything for AVX2 only (or even to SSSE3).

Not sure if we mean the same thing, I'm talking about changing the blind replacement of #cmakedefine to #define to selective based on hardware. This translates to an if-else at the python script #cmakedefine -> #define only if available, otherwise remove the line (don't define).

This is assuming the moz build system doesn't use cmake (which configures #cmakedefine) and the python script is meant to overcome the shortcoming.

Comment 7

[Abhishek]

Jerin, we understand what you are trying to say and it will be fixed.

Comment 8

[Abhishek]

All the build failures are pertaining to the compiler not being able to compile

• AVX512 VNNI code on Linux builders
• AVX512 VNNI, AVX512 and AVX2 code on Windows builders

Mac builders are doing fine.

We will need AVX512 VNNI support in compilers on Linux and Windows builders as well if we want to utilize the latest hardware during translation.

Comment 9

[Jerin Philip]

Hello,

Please note that this will be a problem for ruy when we bring in ARM as well. Intgemm at least causes build to fail, which I consider is nicer than a silent switch. I had to spend some time figuring out the cause of a silent switch into a lower path on ruy at https://github.com/jerinphilip/arm-playground/pull/5#issuecomment-985707343.

Please do have a look at the compiler version requirements (across MSVC, GCC and CLANG) set at the following lines for a reference:

• https://github.com/google/ruy/blob/8c3fd3f266b4a22d542d4aa41329b5018d6b87e1/ruy/platform.h#L100-L159

Comment 10

[Abhishek]

Thanks Jerin. This will be helpful once we start landing Arm implementation.

Comment 11

[Abhishek]

I did a test and ran thorough benchmarks using this code and my intrinsic implementation and I confirm that it works (more details in the comment).

Comment 12

[Yury Delendik (:yury)]

This bug and bug 1746631 is a part of the solution to speed up the translation engine. Writing our own library in support of intrinsic functions does not make any sense at this moment -- the intgemm provides with good implementations for SSE3 and AVX2.

Additional (security?) review of intgemm code base is preferable, but even without it, we are sandboxing it well by Wasm and JavasScript code, the intgemm functionality is stateless, and intrinsic functionality is not accessible by general web applications.

We can land the intgemm code as a third-party one, and the bug 1746631 stays as the only way to enable/access the intgemm functionality.

Comment 13

[André Natal]

Thanks yury. So maybe we can land this without sec review given the level of sandboxing? I had a conversation with @dveditz a month ago and he mentioned we need to make sure the intrinsics and the library are exposed only to privileged or bergamot code only preferable, and that some fuzzy tests would be necessary to be set by the secops fuzzy team. Abhi, what you think?

Comment 14

[Abhishek]

I am not really sure what all steps sec review entails. But the bug 1746631 stays the only way to enable/access the intgemm functionality and this feature is well sandboxed. I believe this satisfies @dveditz concern. I am for landing it without sec review.

Btw, is it technically possible to restrict this feature to be used only by Bergamot privileged extension? Who will be able to answer this? @ryan @yury?

Will these fuzzy tests be added by secops fuzzy team separately? Any idea on whom should we communicate/talk to regarding making this team aware of this feature so that they can add these fuzzy tests?

Comment 15

[Yury Delendik (:yury)]

Btw, is it technically possible to restrict this feature to be used only by Bergamot privileged extension? Who will be able to answer this?
Will these fuzzy tests be added by secops fuzzy team separately? Any idea on whom should we communicate/talk to regarding making this team aware of this feature so that they can add these fuzzy tests?

Let's open a separate bug for that is a blocker for this and 1746631 to address any security concerns here. Theoretically this shall not really stop us from landing entire functionality (at least pref'd off). All security concerns shall be resolved at the point of release and shall not stop us from iterating on Bergamot. I think there is a consensus that while we cannot guarantee that intgemm doing everything 100% safely, but it is contained and it is possible to address this issue in some way in the future.

Comment 16

[Lars T Hansen [:lth]]

To sum up:

• it is fine to land the code preffed-off before security review is complete
• one might further discuss what the criteria are for enabling in Nightly
• under no circumstances can anything ship in any Release channel without an explicit OK from the security people
• the intgemm code shall be reviewed by the spidermonkey team in addition to the security people to ensure correctness and memory safety
• we will need to talk to the fuzzing / testing people (well, really :decoder) about fuzzing and asan runs and how to accomplish them for this weird case

There's a fair amount of talk above about how the "sandbox" makes things less sensitive. I'm not sure what this relates to; intgemm is native code that runs in the content process and is as sensitive as anything else in the browser that's written in C++.

Comment 17

[André Natal]

@Lars I've updated the patch to only be built in Nightly and added a notice that this shouldn't make it to release without further review.

Since this only contains code that touches the building system without anything in the js layer that actually calls it, we can introduce a new pref like we have for the wormhole in the wasm intrinsics patch. Would that work for you?

Currently there's no plans for the translations functionality to be shipped in the release channels, and the scope of usage is only the user study due the EU grant obligations, an eventual internal foxfooding (still without a defined date to start), and early adopters willing to enable it by switching the prefs, always in Nightly. If this ever makes to release, of course we'll request formal security and your team's review, along platform review as well, I suppose.

Since I can't r+ my own patches and given the statement that we can proceed without formal sec-review, I've replaced the former reviewers on phab by you and yury and added firefox-build-system-reviewers since we needed to upgrade the clang and gcc on tc linux jobs.

Try is here: https://treeherder.mozilla.org/jobs?repo=try&revision=844061871776227bdefff1c7b85cf19077cd1695

Please let me know what you think.

Comment 18

[Lars T Hansen [:lth]]

A pref in about:config would be fine with me.

I've r+'d the patch though of course we should wait for everyone to weigh in. As noted in a comment on the patch, pls check with Mike Hoye (or whoever has that job now) whether we need to include mention of the code + the license in the Firefox license blob.

Comment 19

[Abhishek]

@Lars Wouldn't the pref javascript.options.wasm_moz_intgemm which was added via D120662 be enough for this purpose (source code here)?

Comment 20

[Lars T Hansen [:lth]]

Yeah, should be fine.

Comment 21

[André Natal]

Hello Mike, could you please provide us some guidance in terms of licensing for this 3rd party module we are going to merge on gecko-dev? Is it fine to use as is or we need to merge its license to be displayed on about:license?

It will remain in Nightly for an undefined amount of time by now and its source is here: https://github.com/kpu/intgemm

Comment 22

[André Natal]

@nick @glandium I'm flagging you both since I see you are part of the firefox-build-system-reviewers on phab.

We touched a couple of taskclusters jobs to make this build, so we just need to make sure it won’t break anything.

Comment 23

[Mike Hoye [:mhoye]]

In terms of license compliance, there's nothing blocking us here. The main code is under the MIT regime and its dependencies are all BSD-alikes. The only notable comment I'd make here is that when adding it to our licenses file: we've already got a license for Boost in there somewhere, so I'd add "the files in intgemm/test/3rd_party/" to the list of files the Boost license covers rather than adding it again.

Comment 24

[André Natal]

@glandium We removed the tc jobs version bump. Please lmk if is good to go now. Just a reminder: this should remain in nightly and that's enforced by the directives on moz.build files.
@mhoye I did not find the boost licenses you mentioned, at least not on about:licenses. Is it located somewhere else?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4959d55a439a
Adding vendor intgemm library for Firefox Translations r=yury,lth,firefox-build-system-reviewers,glandium

Comment 26

[Noemi Erli[:noemi_erli]]

Backed out changeset 4959d55a439a (Bug 1722102) for causing SM bild bustage CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=dBluTiU8RaW9x8hVUOPW4w.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2Cx64%2Copt%2Cspidermonkey%2Cbuilds%2Cspidermonkey-sm-package-linux64%2Fopt%2Cpkg&revision=4959d55a439a89768001539a82befd6d360c610f

Log: https://treeherder.mozilla.org/logviewer?job_id=364008912&repo=autoland&lineNumber=89640

Backout: https://hg.mozilla.org/integration/autoland/rev/3f9732d3b426ae96289f73e37c256def56132cd5

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e3409252764
Adding vendor intgemm library r=yury,lth,firefox-build-system-reviewers,glandium

Comment 28

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/2e3409252764