1672626 - Warp: transpile ToBool ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P2)

Description

[Jan de Mooij [:jandem]]

Warp currently falls back to MTest/LTestVAndBranch if it doesn't know the type and that's pretty inefficient because it has to test every Value type.

This is a bit non-trivial because:

• Baseline currently has a fast path for booleans, we should replace that with an IC stub.
• MTest is a control instruction. It's probably easiest to refactor this so that MTest always takes a boolean (result of the transpiler or a new MToBoolean instruction). This may be easier to do after removing IonBuilder.
• JSOp::Not also uses the ToBool IC.

Comment 1

[Iain Ireland [:iain]]

Looked into this a bit. I think the biggest obstacle is the second point: MTest currently combines the comparison and the branch in a single instruction. This makes it easier to have good codegen: for example, cmp/jne, which passes the result of the comparison to the branch using the condition flags instead of putting it in a register. If we split MToBoolean and MTest into separate ops, then it seems we would either have to do careful handling of condition codes between LIR instructions (to avoid clobbering the flags) or write more code to fuse them together later.

It seems like we already get pretty good code for an MTest if we know the type of the input; the advantage of transpiling would be cases where the input type is constant, but can't be determined from context. I think the easiest way to handle this is to transpile the conversion, then unconditionally feed the result into the existing MTest and let the optimizer clean it up. For example, if the input is always a string, then we'll get something like:

MUnbox (string)
MNot
MNot
MTest

And then the optimizer will see an MTest with double-negation and fold away the MNots, leaving us with a single guard and a typed MTest. The same approach also works for MNot.

I've got a prototype working; I just have to convert the bool fastpath to an IC, because otherwise we can have bailout loops.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1672626: Remove fastpath for bool in baseline ToBool ICs r=jandem

Removing the dynamic fastpath for bools in BaselineCodeGen allows us to transpile ToBool ICs without causing bailout loops, and gives more accurate frequency information in polymorphic cases.

The baseline compiler still has a static fastpath when the stack value is a known type. That's fine: if baseline can figure out the type, Warp can too, so we don't need CacheIR data.

I took a look at the code we generate for the new IC (on x64):

addl   $0x1,0x8(%rdi)
mov    %rcx,%r11
shr    $0x2f,%r11
cmp    $0x1fff2,%r11d
jne    0x336299e1b4dc
mov    %rcx,%rax
mov    %rax,%rcx
retq

It's pretty good, except that we move the input from rcx to rax (to free up the output register) and then move it right back. We don't currently have a clean way to avoid this, though.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1672626: Transpile ToBool ICs r=jandem

In emitTruthyResult, convertToBoolean wraps the input in MNot->MNot. Since we're feeding into an MTest or an MNot, this gets folded away to nothing.

Depends on D116659

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/123c17cfaaf6
Remove fastpath for bool in baseline ToBool ICs r=jandem
https://hg.mozilla.org/integration/autoland/rev/b46d79f59471
Transpile ToBool ICs r=jandem

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/123c17cfaaf6
https://hg.mozilla.org/mozilla-central/rev/b46d79f59471