Open Bug 813852 Opened 12 years ago Updated 2 years ago

Should boolean operations have MIR opcodes?

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

Details

I don't see obvious MIR opcodes for && and ||.  Should there be some?  Seems like if we had them and we made the corresponding ops movable (which they totally are) we could do some more CSE/LICM magic that we don't do right now.
Ccing Tom, since he's added MIR ops recently.  ;)

I think I have a good handle on what I'd need to implement for the op itself to make it movable and such, but there seem to be fiddly pieces here...
(In reply to Boris Zbarsky (:bz) from comment #0)
> I don't see obvious MIR opcodes for && and ||.  Should there be some?

Because SSA forms does not work that way, and the semantic of && is to skip the execution of the right hand side if the left operand is false.  These opcodes appear as the last instruction of a block.

> Seems
> like if we had them and we made the corresponding ops movable (which they
> totally are) we could do some more CSE/LICM magic that we don't do right now.

Knowing that they are the last instruction of op-code, moving them is quite complex, because we would need to duplicate the blocks that they are traversing on each successors.  Moving them above loops will imply to duplicate the whole loop under each successor.  The duplication might cost a lot in terms of register allocation, and this is the kind of thing that we should avoid if there is a small perf gain.  Doing CSE is equivalent to removing all successors except one.

I think the first step before starting to duplicate blocks would be to check the number of Compare instruction which are hoisted far from there Test instruction.  CSE might be easy to do if we can evaluate the Compare instruction ahead of time, and detect that the boolean input of the Test instruction is a constant.
Oh, right.  I forgot about the short-circuiting behavior.  That's... annoying.  :(
Assignee: general → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.