Wasm: codegen for trunc and nearest with more tests

[commonlisp]

This change is

[msftclas]

Hi @commonlisp, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[MikeHolman]

this is a bit confusing now. i think would be more clear what is going on if we have switch on m_opcode (and i'm pretty sure || instr->m_opcode != Js::OpCode::InlineMathFloor will always be false)

[MikeHolman]

was this block dead before?

[commonlisp]

As far as I can tell. InlineMathRound and InlineMathFloor (i.e., != InlineMathCeil) are both handled by the first if-clause. Thus, the only remaining case is InlineMathCeil for which != InlineMathFloor always evaluates to true.

[Cellule]

I double checked and this is correct. The last block was never used.

[Cellule]

Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

lib/Backend/Lower.cpp, line 2989 [r2] (raw file):

        case Js::OpCode::Trunc_A:
            m_lowererMD.GenerateFastInlineBuiltInCall(instr, (IR::JnHelperMethod)0);

This requires SSE 4, it will crash or produce invalid code if the machine doesn't have sse 4.
You can test with ch.exe misc.js -on:wasm -sse:2 -forcenative
Either make a valid path for this case or make it NYI in this case like so
WASM_UNARY__OPCODE(F32Trunc, 0x80, F_F , Trunc_Flt , !AutoSystemInfo::Data.SSE4_1Available())

---

lib/Backend/LowerMDShared.cpp, line 6136 [r2] (raw file):

void
LowererMD::GenerateTrunc(IR::Instr * instr)

Also remove the declaration of both functions in the header file

---

test/wasm/misc.wast, line 6 [r2] (raw file):

;;-------------------------------------------------------------------------------------------------------

(module

Add f64 tests for trunc and nearest. Also use named functions to avoid potential errors with the index when exporting

---

Comments from Reviewable

[MikeHolman]

lib/Backend/Lower.cpp, line 2989 [r2] (raw file):

Previously, Cellule (Michael Ferris) wrote…

This requires SSE 4, it will crash or produce invalid code if the machine doesn't have sse 4.
You can test with ch.exe misc.js -on:wasm -sse:2 -forcenative
Either make a valid path for this case or make it NYI in this case like so
WASM_UNARY__OPCODE(F32Trunc, 0x80, F_F , Trunc_Flt , !AutoSystemInfo::Data.SSE4_1Available())

Good catch, this definitely needs a code path for older machines. IMO we shouldn't have this defined conditional on SSE4.1 because that will mess up testing, e.g. in case the test VM has or does not have SSE4.1

---

Comments from Reviewable

[commonlisp]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

lib/Backend/Lower.cpp, line 2989 [r2] (raw file):

Previously, MikeHolman (Michael Holman) wrote…

Good catch, this definitely needs a code path for older machines. IMO we shouldn't have this defined conditional on SSE4.1 because that will mess up testing, e.g. in case the test VM has or does not have SSE4.1

For now, using builtins for SSE2. This is consistent with how InlineMathCeil and Floor are being handled right now.

---

lib/Backend/LowerMDShared.cpp, line 6136 [r2] (raw file):

Previously, Cellule (Michael Ferris) wrote…

Also remove the declaration of both functions in the header file

Done.

---

test/wasm/misc.wast, line 6 [r2] (raw file):

Previously, Cellule (Michael Ferris) wrote…

Add f64 tests for trunc and nearest. Also use named functions to avoid potential errors with the index when exporting

Done.

---

Comments from Reviewable

[Cellule]

---

Reviewed 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable