Closed Bug 1770335 Opened 3 months ago Closed 2 months ago

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

Categories

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

Firefox 100
defect

Tracking

()

RESOLVED FIXED
103 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox100 --- wontfix
firefox101 --- wontfix
firefox102 --- fixed
firefox103 --- fixed

People

(Reporter: aous72, Assigned: yury)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.

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.

Component: Untriaged → Javascript: WebAssembly
Product: Firefox → Core

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.

Flags: needinfo?(ydelendik)

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

Regressed by: 1672343
Status: UNCONFIRMED → NEW
Ever confirmed: true

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

Assignee: nobody → ydelendik
Severity: -- → N/A
Priority: -- → P2
Attached file WIP: Bug 1770335 - Revert bug 1672343 (obsolete) —

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.

Flags: needinfo?(ydelendik)
Has Regression Range: --- → yes

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(rhunt)
Severity: N/A → S2
Flags: needinfo?(rhunt)
Attached file test-case.zip for jsshell (obsolete) —

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.

Attachment #9277573 - Attachment is obsolete: true
Attachment #9279909 - Attachment is obsolete: true

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.

Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89eb8f006446
Fix MWasmShuffleSimd128::congruentTo logic. r=jseward
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

Is this something we should consider for Beta uplift?

Flags: needinfo?(ydelendik)
Flags: in-testsuite+

Is this something we should consider for Beta uplift?

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

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

Flags: needinfo?(ydelendik)

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
Attachment #9280134 - Flags: approval-mozilla-beta?

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

Approved for 102 beta 7, thanks.

Attachment #9280134 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Thank you guys for the excellent work.

You need to log in before you can comment on or make changes to this bug.