Closed Bug 1255691 Opened 4 years ago Closed 4 years ago

wasm: select

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: sunfish, Assigned: bbouvier)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch select.patch (obsolete) — Splinter Review
Select is not in asm.js, so current asm2wasm code doesn't use it, but the LLVM wasm backend does use it.
Attached patch select.patch (obsolete) — Splinter Review
As discussed with Dan on irc, taking over. I've got a basic impl for integer select on x86 with cmovz/cmovnz.

Remains to do:
- select for other types on x86 shared
- on ARM, it should be quite nice to be able to use the condition codes (~cmov)
- lots of tests (for testing foldsTo, in particular)
- maybe random small optimizations: if the condition already is a comparison, flags are already set and no need to test(cond, cond).
Assignee: sunfish → bbouvier
Status: NEW → ASSIGNED
Attachment #8729337 - Attachment is obsolete: true
Comment on attachment 8730859 [details] [diff] [review]
select.patch

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

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +313,5 @@
> +
> +    if (mirType == MIRType_Int32) {
> +        masm.test32(cond, cond);
> +        masm.cmovnz(trueExpr, out);
> +        masm.cmovz(falseExpr, out);

This works, but ideally we'd like to do this with only one cmov. One way to do this would be to use defineReuseInput at lowering time, so that one of the inputs is already in the output register, and it just needs one cmov to conditionally overwrite it with the other input, though that will likely require lowering to be target-specific.

Another way would be to do the following: If the trueExpr register happens to be the same as the output register, cmovz the falseExpr to the output. If the falseExpr register happens to be the same as the output register, cmovnz the trueExpr to the output. Otherwise, it's safe to just copy one of the inputs to the output, and then cmov to conditionally overwrite it with the other.
Attached patch select.patch (obsolete) — Splinter Review
Good suggestion! Actually I think that defineReuseInput can work on all platforms: on ARM, we can transform all moves into "cmoves". Will work on that tomorrow; the patch has been tested on x64/x86 and it works nicely, even with int64.

I haven't implemented SIMD selects, as 1. there are special SIMD select instructions, 2. there's no way to test it or run into it while fuzzing.

I am not so sure about int64 select, though; reusing the same instruction as for int32 worked, so I imagine the cmov I've implemented is actually the 64 bit version. Will check this tomorrow as well.
Attachment #8730859 - Attachment is obsolete: true
Attached patch select.patchSplinter Review
Done! Making moves conditional on ARM did the trick. I've removed the foldsTo optimizations as it was wrong (it was returning trueExpr/falseExpr depending on whether the condition was true/false; but that wasn't executing side effects of the removed expression! I don't think foldsTo can return an expression while adding another one to the same basic block without messing up GVN, so I just removed it entirely).

Lowering is platform dependent, because x86-shared can use use()/Operand for the falseExpr, while ARM uses useRegister()/Register/FloatRegister for the falseExpr.
Attachment #8731367 - Attachment is obsolete: true
Attachment #8731655 - Flags: review?(sunfish)
Comment on attachment 8731655 [details] [diff] [review]
select.patch

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

::: js/src/jit/shared/LIR-shared.h
@@ +7221,5 @@
>          return mir_->toHasClass();
>      }
>  };
>  
> +template<size_t numDefs, size_t numOps>

Locally renamed to Defs/Ops, because numDefs() is a function in LInstructionHelper and msvc didn't like that.
Comment on attachment 8731655 [details] [diff] [review]
select.patch

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

Looks good!

::: js/src/jit-test/tests/wasm/basic.js
@@ +504,5 @@
> +            case "nan": return NaN;
> +            case "-0": return -0;
> +            default: return val;
> +        }
> +    }

What is the purpose of allowing some values to be passed in as strings, instead of just passing in the Infinity, NaN, -0 directly?
Attachment #8731655 - Flags: review?(sunfish) → review+
Thank you!

> 
> What is the purpose of allowing some values to be passed in as strings,
> instead of just passing in the Infinity, NaN, -0 directly?

For every other value than "Infinity", "NaN", the value can be expressed the same in wasm and in JS:

(module (func (f32.const ${value})))

assertEq(x, value);

In wasm, NaN is "nan" and Infinity is "infinity", so we need to special case those. For -0, it was to be sure that -0 is passed as negative 0 and not converted as just 0 when implicitly calling toString() in the template string.
https://hg.mozilla.org/mozilla-central/rev/32f3f27d070c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.