744431 - IonMonkey: Remove all possible negative zero cases

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Attachment: Concept

Follow-up on bug 739854 and bug 736135.

This will replace the logic added in those bugs. The algorithm used removed mostly, but not all negative zero's. This because I thought it would be to intensive/complicated to compute them all. Guess I was wrong, because it is possible to do this in two passes. (Forward and backwards pass).
(E.g. on ((x+y)+z)+1 only the negative zero on x or y was removed)

In attachment I've added the necessary logic to do this. I just wrote it out of head, so there could be minor issues in it, but the idea works definitely.

I'm sorry, but I probably won't be able to finish/implement it immediately as I should focus on my studies now. Yeah it's that time of the year. I've added this, so nobody else have to wonder if it is possible and how. I expect I can add it begin July, hopefully sooner. If somebody understands the code, he may add it too. But I don't want to force somebody to do it. The implementation so far is good enough for now.

The concept is just almost c++ code containing the hard cases. So all range analysis code and the code for MAdd/MMul/MSub. All other instruction that still need to get added are straightforward.

Comment 1

[Hannes Verschore [:h4writer]]

@Marty: I know you already said it was possible and I have no idea if you have already made some code. If you haven't it is not needed to create it again ...

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Concept V1.1

Simplify code a bit, especially the functions added to MInstruction. They are now easier to understand and to implement.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part 1 - Remove range analysis

Removes old range analysis logic

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part2 - Partially readds range analysis

This is a partially patch containing only logic for MMul, MDiv, MToInt32 and MAdd. It successful removes all negative zero checks of "a*c+b*b", "a*b+a*c*b*c+1" ...
Cases that the old logic couldn't remove!

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Part 2 - Readd range analysis WIP

This adds all logic of the old range analysis pass again and new parts.

For V8 we remove now 372 of 1148 bailouts. (old pass did only 100-200)
SS shows a small increase in performance 1ms-2ms, didn't try others yet.

The new logic now makes it possible to remove negative zeros over phis/double operations/...

And mostly the reason why a negative zeros can't get removed is now Return/callsetproperty/passarg. So it looks like we are getting to an endpoint. I think I can remove a couple more, but that's it. To remove even more negative zero checks we will have to propagate over function. So that's definitely future work!

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore from comment #5)
> For V8 we remove now 372 of 1148 bailouts. (old pass did only 100-200)
> SS shows a small increase in performance 1ms-2ms, didn't try others yet.
s/bailouts/negative zero checks/

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Readd range analysis WIP1.1

Corrected propagation of negative zero with Phis. In loopheaders we don't always see the definitions before the uses. So I removed propagation on those phis. Technically it would be possible, by doing multiple sweeps over the code. 
Also fixed another issue. When RangeAnalysis pass computed a value couldn't propagate an negative zero it didn't set the behaviour_ correctly. That was the reason for assert on debug.

Patch is starting to look complete. Just have to check the logic for canBeNegativeZero if those hold for doubles.

The count of removed negative zero's in v8 is now 247 out of 564 (The lower count of instruction where a negative zero check can get removed is because I now only count the instructions specialized to integer)

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.