1770335 - My WASM SIMD code behaves correctly only when wasm_optimizingjit is false.

Status: RESOLVED FIXED

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

Description

[Aous Naman]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:100.0) Gecko/20100101 Firefox/100.0

Steps to reproduce:

Visit the webpage https://openjph.org/javascript2/demo.html
with default Firefox settings --> you get rubbish images.

Now set javascript.options.wasm_optimizingjit to false --> you get correctly rendered images.

The images are rendered correctly on Chrome and Edge (I know they both use the same engine).

The demo.html has a javascript code that detects browser supports for wasm simd. Then, it dynamically load generic wasm code or wasm with simd code. The generic wasm code runs without a problem.

I think wasm_optimizingjit flag changes the generated code.

Actual results:

Rubbish images with javascript.options.wasm_optimizingjit = true
Correct images with javascript.options.wasm_optimizingjit = false

Expected results:

Correctly rendered images in both cases.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Javascript: WebAssembly' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Lars T Hansen [:lth]]

Yury, maybe you can take a look? I need to restart after flipping the pref, btw, but the page does look fine with baseline-only.

Same problem on ARM64, so it's not low-level instruction selection per se.

Comment 3

[Mayank Bansal]

2022-05-20T17:41:46.487000: DEBUG : Found commit message:
Bug 1672343 - Ignore one of the SIMD shuffle operands if it is not used. r=lth

Differential Revision: https://phabricator.services.mozilla.com/D124382

2022-05-20T17:41:46.487000: DEBUG : Did not find a branch, checking all integration branches
2022-05-20T17:41:46.502000: INFO : The bisection is done.
2022-05-20T17:41:46.503000: INFO : Stopped

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1672343

Comment 5

[Yury Delendik (:yury)]

Attachment: WIP: Bug 1770335 - Revert bug 1672343

Comment 6

[Yury Delendik (:yury)]

I can confirm that reversing bug 1672343 fixes the issue. I'll throw WIP for reversing. Though I not really sure why, and the function is huge to quickly spot the effect of the change. I will keep investigating why ignoring the zero constant in some instructions causing function to misbehave later in the code.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1672343

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rhunt, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Yury Delendik (:yury)]

Attachment: test-case.zip for jsshell

Progress so far: made a test case to run with jsshell. With help of bug 1728547, identified failing function: "wasm-func79". It is a huge function, working on identifying the problem.

Comment 10

[Yury Delendik (:yury)]

Attachment: Bug 1770335 - Fix MWasmShuffleSimd128::congruentTo logic. r?jseward

The implementation was incomplete. It is possible to have the same
control data field in the SimdShuffle. The ValueNumberer algorithm
wrongfully merging non-congruent values.

Compare all SimdShuffle fields, including opd, permuteOp, and shuffleOp.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89eb8f006446
Fix MWasmShuffleSimd128::congruentTo logic. r=jseward

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/89eb8f006446

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta uplift?

Comment 14

[Julian Seward [:jseward]]

Is this something we should consider for Beta uplift?

I would be in favour of that. But it's Yury's call.

Comment 15

[Yury Delendik (:yury)]

I can do that, yes. I mostly wonder about ESR though.

Comment 16

[Yury Delendik (:yury)]

Comment on attachment 9280134 [details]
Bug 1770335 - Fix MWasmShuffleSimd128::congruentTo logic. r?jseward

Beta/Release Uplift Approval Request

• User impact if declined: It is correctness of executed wasm SIMD code. Some programs may generated incorrect result.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minimal test is provided; the patch touches only broken wasm SIMD code instructions behavior.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9280134 [details]
Bug 1770335 - Fix MWasmShuffleSimd128::congruentTo logic. r?jseward

Approved for 102 beta 7, thanks.

Comment 18

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/247d3134bbd5

Comment 19

[Aous Naman]

Thank you guys for the excellent work.