1762899 - WASM C standard library atan2(-1,0) is returning pi/2 instead of -pi/2

Status: RESOLVED FIXED

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

Description

[Ryan Hunt [:rhunt]]

Source: https://github.com/mozilla-mobile/fenix/issues/24565

According to RyanVM, this still reproduces on his Pixel 3 with Fenix Nightly.

Yury, can you take a look?

Comment 1

[Yury Delendik (:yury)]

I can reproduce on arm64 simulator.

Comment 2

[Yury Delendik (:yury)]

Attachment: Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r?lth

Comment 3

[Yury Delendik (:yury)]

The guess about copysign was the correct one: in case when rhs == output in MacroAssembler::copySignDouble we were overwriting rhs by lhs, so the operation becomes noop. The simple test case to reproduce is:

  (func (param f64 f64) (result f64)
    f64.const 0x1.921fb54442d18p+0 (;=1.5708;)
    local.get 0
    f64.copysign
  )

Comment 4

[Yury Delendik (:yury)]

Attachment: Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r?lth

Using defineReuseInput in LIRGenerator to avoid rhs == output case in copySignDouble().

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62b1090431b0
Fix {f32,f64}.copysign for Aarch64. r=lth

Comment 6

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/62b1090431b0

Comment 7

[Yury Delendik (:yury)]

Comment on attachment 9270975 [details]
Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r?lth

Beta/Release Uplift Approval Request

• User impact if declined: Some wasm application may not work properly on Aarch64 or Apple M1 hardware.
• 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): The changes only affect the code that produces the error, limited to the affected platform, and tests are provided.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Having the proper implementation of Wasm SIMD is somewhat important -- new application that use this technology are appearing on the market at this moment and could cause issues for ESRs.
• User impact if declined: Some wasm application may not work properly on Aarch64 or Apple M1 hardware.
• Fix Landed on Version: 101
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes only affect the code that produces the error, limited to the affected platform, and tests are provided.

Comment 8

[Dianna Smith [:diannaS]]

Comment on attachment 9270975 [details]
Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r?lth

Approved for 100.0b3

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9270975 [details]
Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r?lth

Important ARM64 WASM correctness fix with tests. Approved for 91.9esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/554ce378af3d

Comment 11

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/89ce78c2fdfa

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9270975 [details]
Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r?lth

Approved for 99.0.1 and Fenix/Focus 99.2.0

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/fea37b0b71f2