1255691 - wasm: select

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

Attachment: select.patch

Select is not in asm.js, so current asm2wasm code doesn't use it, but the LLVM wasm backend does use it.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: select.patch

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).

Comment 2

[Dan Gohman [:sunfish]]

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.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: select.patch

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.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: select.patch

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.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

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 6

[Dan Gohman [:sunfish]]

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?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32f3f27d070c

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32f3f27d070c