1437039 - [MIPS32] - Simplify 64-bit branching

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Milan Knežević]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.119 Safari/537.36 OPR/51.0.2830.26

Comment 1

[Milan Knežević]

Attachment: 64bit-branches.diff

The branch64 method's logic is simplified and moved to cmp64Set.

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

Milan, is the patch ready for review yet?

Comment 3

[Milan Knežević]

Attachment: 64bit-branches.diff

Yes, patch is ready for review now.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8950070 [details] [diff] [review]
64bit-branches.diff

Review of attachment 8950070 [details] [diff] [review]:
-----------------------------------------------------------------

Dragan, would you be the right person to review these patches?

Comment 5

[Dragan Mladjenovic]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8950070 [details] [diff] [review]
> 64bit-branches.diff
> 
> Review of attachment 8950070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Dragan, would you be the right person to review these patches?

I rather have a third party to review this first.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8950070 [details] [diff] [review]
64bit-branches.diff

Review of attachment 8950070 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!  This is obviously the best fix also for bug 1316822, so once you've landed this I'll probably just promote cmp64Set to MacroAssembler.h and implement it properly everywhere.

::: js/src/jit-test/tests/wasm/integer.js
@@ +301,5 @@
>  testComparison64('gt_u', 40, 40, 0);
>  testComparison64('ge_s', 40, 40, 1);
>  testComparison64('ge_u', 40, 40, 1);
>  testComparison64('eq', "0x400012345678", "0x400012345678", 1);
> +testComparison64('eq', "0x400012345678", "0x500012345678", 0);

Thank you for adding more test cases!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +3824,5 @@
>      void cmp64Set(Assembler::Condition cond, RegI64 lhs, RegI64 rhs, RegI32 dest) {
>  #ifdef JS_PUNBOX64
>          masm.cmpPtrSet(cond, lhs.reg, rhs.reg, dest);
>  #else
> +# if defined(JS_MIPS32)

Can you change the ifdef above to if defined, and change this to an elif?

Comment 7

[Lars T Hansen [:lth]]

Oh, and btw, that probably needs to be JS_CODEGEN_MIPS32 does it not?

Comment 8

[Milan Knežević]

Attachment: 64bit-branches_14022018.diff

Thank you for review, and sorry for my sloppy changes. The patch is changed according to your comments.

Comment 9

[Lars T Hansen [:lth]]

Yeah, looks nice.  Can this land now or is it blocked on other code landing first?

Comment 10

[Milan Knežević]

The wasm baseline code should be landed before this code.

Comment 11

[Milan Knežević]

Attachment: 64bit-branches.diff

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb41cefd7f70
[MIPS32] - Simplify 64-bit branching. r=lth

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fb41cefd7f70