IonMonkey: Implement MathFunction(Floor) recover instruction

RESOLVED FIXED in Firefox 58

Status

()

defect
P5
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: nbp, Assigned: schweerian33, Mentored, NeedInfo)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
Implement MathFunction(Floor) in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

This issue is similar to Bug 1028573.
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Hello,
this is the first bug for me to work on.

I think we only need to add floor in switch case in line 880 in file js/src/jit/Recover.cpp
MMathFunction::writeRecoverData(CompactBufferWriter &writer) const

case Floor:
        writer.writeUnsigned(uint32_t(RInstruction::Recover_Floor));
        return true;

Is that right ?!
Reporter

Comment 11

5 years ago
Comment on attachment 8536211 [details] [diff] [review]
rev 1 - Implement MathFunction(Floor)

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

Can you add a test case and verify that this path is well taken?
Also attach the right MIRGraph phase, as documented in Bug 1003801.

::: js/src/jit/Recover.cpp
@@ +882,5 @@
>      MOZ_ASSERT(canRecoverOnBailout());
>      switch (function_) {
> +      case Floor:
> +        writer.writeUnsigned(uint32_t(RInstruction::Recover_Floor));
> +        return true;  

This sounds correct, except for this trailing white space.
Attachment #8536211 - Flags: review?(nicolas.b.pierron)
Attachment #8537715 - Attachment description: rev 2 - floor number (func 153 pass 16) → rev 2 - floor number (func 152 pass 16)
Reporter

Comment 15

5 years ago
Comment on attachment 8537712 [details] [diff] [review]
rev 2 - Implement MathFunction(Floor) recover instruction

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

This is indeed better, but can you include the test case.
The output of IonGraph is not showing any recovered instruction, nor any MMathFunction.  It has a MFloor but not an MMathFunction(Floor).  Have a look at [1] to see how we produce such MIR Instruction:

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#812-818
Attachment #8537712 - Flags: review?(nicolas.b.pierron) → feedback+
Reporter

Updated

5 years ago
Attachment #8537715 - Flags: review?(nicolas.b.pierron)
Reporter

Comment 16

5 years ago
Comment on attachment 8537717 [details]
rev 2 - floor object (func 153 pass 16)

I see an MFloor, but no MMathFunction.

Also, I think this phase is too early in the pipeline as it does not show any recovered instruction.
Attachment #8537717 - Flags: review?(nicolas.b.pierron)
Comment hidden (obsolete)
Hi Nicolas, I'd like to work on a test case for this, but I'm not clear at what I'm looking at in js/src/jit/MCallOptimize.cpp that you linked.  Could you elaborate?

I also see that dce-with-rinstructions.js already has rfloor_number and rfloor_object.  Would the test cases be different for this?

I'm currently working off of Abdelrhman's patch, which I assume is correct so far, judging from your previous comments.  Thanks!
Reporter

Comment 19

4 years ago
(In reply to Dongie Agnir [:dagnir] from comment #18)
> Hi Nicolas, I'd like to work on a test case for this, but I'm not clear at
> what I'm looking at in js/src/jit/MCallOptimize.cpp that you linked.  Could
> you elaborate?

You should look for the conditions which are used to switch between a MMathFunction(Floor) and a MFloor, and make sure to make a test case for it.

> I also see that dce-with-rinstructions.js already has rfloor_number and
> rfloor_object.  Would the test cases be different for this?

Yes, but only in the inputs/outputs of the floor function, that we recorded while executing code generated by baseline.
Dongie, are you still interested in working on this?
Flags: needinfo?(dongie.agnir)
Priority: -- → P5
Assignee

Comment 21

3 years ago
Hello Till,

Is it possible to work on this? Is someone else assigned?

Ian
Flags: needinfo?(till)
Hi Ian,

given that the last activity here is from 1.5 years ago, I think that is perfectly fine, yes. Thank you for taking this on!

Please ask Nicole Pierron for feedback/review on patches or any further information you might need. You might also be interested in the other bugs blocking bug 1003801. Nicolas also tells me that there's probably more to be done in this area, so please let us know if you want to have additional bugs filed for you to work on.
Flags: needinfo?(till)
Flags: needinfo?(dongie.agnir)
Assignee

Comment 23

3 years ago
Hi Till,

Awesome, thanks so much! Is there a preferred method for communication with other members? I see Nicole is active on 1003801. I briefly read through the issue, I think I understand the use case, but I may need some more guidance. 

Ian
Reporter

Comment 24

3 years ago
(In reply to schweerian33 from comment #23)
> I briefly read through the
> issue, I think I understand the use case, but I may need some more guidance. 

What kind of guidance are you looking for?
Assignee

Comment 25

3 years ago
Hey Nicolas,

Let me make sure I understand the issue. From the description in bug 1003801, we need to generate another set of instructions for the current execution path and move them to the bailout path. How does the bailout path occur? Does this mean that particular javascript instruction gets terminated? There is also a diff located https://bugzilla.mozilla.org/attachment.cgi?id=8408953&action=diff (Is this the proper way to link to diffs in other tickets?). What is this diff doing, and how can it be abstracted to be applied to the floor instruction?

This is my first bug and first major interaction with the codebase. I've read some documentation on IonMonkey, but where is the best source of documentation?

One meta question, are tickets the best place for questions on this issue? Or is there another method of communication preferred by the team?

Ian
Assignee

Comment 26

3 years ago
Hey all,

Sorry for the double comment. Decided to read some documentation on IonMonkey and baseline (https://blog.mozilla.org/javascript/2014/07/15/ionmonkey-optimizing-away was an awesome start). I think I answered my own questions :).
Reporter

Comment 27

3 years ago
(In reply to schweerian33 from comment #25)
> Let me make sure I understand the issue. From the description in bug
> 1003801, we need to generate another set of instructions for the current
> execution path and move them to the bailout path. How does the bailout path
> occur?

Recover instructions are executed in 2 cases, either during a bailout in BailoutIonToBaseline [1], or when we need to introspect the stack [2].

> Does this mean that particular javascript instruction gets
> terminated?

Moved to the Recover-on-bailout path, and no longer executed by IonMonkey, yes.

> (Is this the proper way to link to diffs in other tickets?)

attachment 8408953 [details] [diff] [review]  would be the proper way, then bugzilla pretty print the link and add the […] links next to the "attachment <id>" text of your comment.

> There is also a diff located attachment 8408953 [details] [diff] [review] ([…]). What is this diff doing,
> and how can it be abstracted to be applied to the floor instruction?

To implement MathFunction(Floor), you would have to extend the code that we already have [3], to support MathFunction::Floor, and make sure we can recover MMathFunction MIR instruction if we have a floor instruction.

> This is my first bug and first major interaction with the codebase. I've
> read some documentation on IonMonkey, but where is the best source of
> documentation?

Unfortunately no.
The best option I have to offer is a guide of tips & tricks [4] to debug / hack on SpiderMonkey.

> One meta question, are tickets the best place for questions on this issue?
> Or is there another method of communication preferred by the team?

For everything which is related to the bug understanding and resolution, yes.
For all other question, you can use IRC #jsapi channel [5].

[1] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/js/src/jit/BaselineBailouts.cpp#1514-1516
[2] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/js/src/jit/JitFrames.cpp#1916
[3] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/js/src/jit/Recover.cpp#980
[4] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips
[5] https://wiki.mozilla.org/IRC
Assignee

Comment 28

3 years ago
Awesome, thanks for the help Nicolas!

Could someone assign this ticket to me?

Ian
Reporter

Updated

3 years ago
Assignee: nobody → schweerian33
Assignee

Comment 29

3 years ago
Assignee

Comment 30

3 years ago
Hey Nicolas,

I attached a patch file to see if I could get some early review. I have added some logic in Recover.cpp and was hoping I could get some direction on where to go from here? 

Also, should I be using MozReview for this?

Ian
Reporter

Comment 31

3 years ago
Comment on attachment 8802374 [details] [diff] [review]
ianschweer_progress.patch

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

This patch looks good except that you forgot something in MIR.h.
I suggest you try to make a test case for it, and double check with iongraph[1] if you can get this new code path to be executed.

If you prefer to use patches instead of MozReview, I am fine with both.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_(JS_shell)
Assignee

Comment 32

3 years ago
Hey Nicolas,

Can you assist me in writing a test case? 

Ian
Reporter

Comment 33

3 years ago
(In reply to schweerian33 from comment #32)
> Can you assist me in writing a test case? 

Yes, look for MMathFunction::New created with Floor.
Then look what conditions make you avoid the other optimizations paths, and write your test case such that you avoid these paths.  You can you iongraph to see what it get compiled to instead of being a MathFunction.

Does that make sense?
Do you have any more specific question?
Assignee

Comment 34

3 years ago
I don't understand the condition in MCallOptimize where we call MMathFunction::New for floor. Can you help me understand how to write a test case to enter this block of code?

http://searchfox.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#997
Reporter

Comment 35

3 years ago
(In reply to schweerian33 from comment #34)
> I don't understand the condition in MCallOptimize where we call
> MMathFunction::New for floor. Can you help me understand how to write a test
> case to enter this block of code?
> 
> http://searchfox.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#997

The return value type is the one which should be monitored by Baseline inline caches.  It does not has to always be a double, but this would be easier, it has to be monitored as a double before we enter IonMonkey.

Math.floor returns numbers with no fractional part.  If you want to have a double returned type you need to make the function return either a number which is not in the int32 range, or -0.

As the compiler is clever enough to inline constants, we should ensure that the input of Math.floor is computed from a variable input.

Have you look at http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js#477 ?
Assignee

Comment 36

3 years ago
Yeah, my test case currently is based off [1]. My test case (I will upload it) are the lines 477-482 plus whatever boilerplate was needed, and then gets called with the arguments [0,100]. I put a MOZ_CRASH at line 998 in [2] and I never saw the crash while running the test case, so I'm assuming I'm missing some step.

[1]: http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js#477
[2]: http://searchfox.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#997
Reporter

Comment 37

3 years ago
I did the same experiment with success.

I changed the line with something like:

    var x = Math.floor(i % 2 == 0 ? -0 : i + 0.1111);

And executed the test case with

  $ js --no-threads --ion-eager jit-test/tests/ion/dce-with-rinstructions.js
  Hit MOZ_CRASH(foobar) at /home/nicolas/mozilla/some-dev/js/src/jit/MCallOptimize.cpp:998
Assignee

Comment 38

3 years ago
I don't know what I did to not make this work but I got the MOZ_CRASH to hit, and I think I should be able to find the change in MIR.h. I hope to have it to you before the Thanksgiving holiday coming up.

Comment 39

2 years ago
Is this still an open bug? Can i pick it up?
Are you still working on it? Or can Malik take over?
Flags: needinfo?(schweerian33)
Assignee

Comment 41

2 years ago
Hi malik, Hannes,

No, I'm not working on it anymore. Please feel free to take it. I can assign it to whoever.

Ian
I'll take this one. I've built Spidermonkey, and I'm going over the explanation detailed in Bug 1003801

Comment 43

2 years ago
Posted patch math-floor.patch (obsolete) — Splinter Review
Attachment #8925372 - Flags: review?(nicolas.b.pierron)

Comment 44

2 years ago
I'm still pretty new here. A few questions:

1. What's the difference between MathFunction::Floor and Floor? Is it just that Floor is called on numbers, and MathFunction::Floor is called on doubles?

2. This patch doesn't actually work as-is on my machine. I had to comment out [1] for my machine to actually run the MathFunction instructions. Is there a more elegant way to disable functionality on my architecture for testing?

Bonus questions, just for fun:

3. Are we generating actual factual assembly code? Like, is there an x86 "RoundsDown" instruction that this code is checking for?

4. If we *are* generating actual assembly code from JavaScript, does this mean that we're JIT-ing WebAssembly and "vanilla" JavaScript with the same code paths?

5. I tried to submit this with MozReview, but got the error

    jhemphill@macbook-pro-59 ~/o/f/j/s/build_DBG.OBJ> hg push review
    pushing to https://reviewboard-hg.mozilla.org/autoreview
    searching for appropriate review repository
    redirecting push to https://reviewboard-hg.mozilla.org/gecko
    searching for appropriate review repository
    abort: no review repository found

Is there some recursive searching going on? I thought everything in firefox-central belonged to one repository.

[1] https://hg.mozilla.org/mozilla-central/file/e64846245b00/js/src/jit/MCallOptimize.cpp#l1115
Reporter

Comment 45

2 years ago
(In reply to jthemphill from comment #44)
> I'm still pretty new here. A few questions:
> 
> 1. What's the difference between MathFunction::Floor and Floor? Is it just
> that Floor is called on numbers, and MathFunction::Floor is called on
> doubles?

The difference is that MFloor would be used for cases where the result generates an int32. This is not guaranteed for larger values such as (2^42 + 0.25).  In particular MFloor generates some code to do the round-down [1], while Mathfunction::Floor produces a function call [2].

Maybe it would make sense to use recover instructions for MNearbyInt as well, in addition to MFloor.

[1] http://searchfox.org/mozilla-central/source/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp#1930
[2] http://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#7078

> 2. This patch doesn't actually work as-is on my machine. I had to comment
> out [1] for my machine to actually run the MathFunction instructions. Is
> there a more elegant way to disable functionality on my architecture for
> testing?

We used to have a command line argument for that. Maybe you can try with --no-sse3 or --no-sse4.

> 3. Are we generating actual factual assembly code? Like, is there an x86
> "RoundsDown" instruction that this code is checking for?

Yes. Have a look at the various files named *Assembler* under js/src/jit.

> 4. If we *are* generating actual assembly code from JavaScript, does this
> mean that we're JIT-ing WebAssembly and "vanilla" JavaScript with the same
> code paths?

Yes. This historically comes from the fact that AsmJS is a subset of JavaScript, and that WebAssembly is inspired by AsmJS, and so were our implementation as well.

> 5. I tried to submit this with MozReview, but got the error
> 
> […]

I have no clue about this tool, maybe ask on irc.mozilla,org, in the #introduction channel.

Note, I will review the patch tomorrow, sorry for the delay.
Reporter

Comment 46

2 years ago
Comment on attachment 8925372 [details] [diff] [review]
math-floor.patch

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

Make it work on your machine, either by implementing recover instruction for MNearbyInt instruction, or by moving this test to a different file and add the --no-sse3 option on the first line of the test case:

http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/baseline/bug1095870.js#1
Attachment #8925372 - Flags: review?(nicolas.b.pierron) → feedback+

Comment 47

2 years ago
Attachment #8925372 - Attachment is obsolete: true
Attachment #8926284 - Flags: review?(nicolas.b.pierron)

Comment 48

2 years ago
I think this works!

```
% js/src/jit-test/jit_test.py js/src/build_DBG.OBJ/dist/bin/js rinstructions-no-sse4.js
[1|0|0|0] 100% ==========================================================>|   0.4s
PASSED ALL
```

I'm happy to implement MNearbyInt in a separate patch. Should I open a new bug?
Reporter

Comment 49

2 years ago
Comment on attachment 8926284 [details] [diff] [review]
floor-no-sse4.patch

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

Great! This patch is perfect, I will send it to the try server to double check it, and push it to mozilla-inbound once the try run is green.
Attachment #8926284 - Flags: review?(nicolas.b.pierron) → review+
Reporter

Comment 50

2 years ago
(In reply to jthemphill from comment #48)
> I'm happy to implement MNearbyInt in a separate patch. Should I open a new
> bug?


Sure, add me in the CC list.
Reporter

Comment 51

2 years ago
The patch builds fine on the Try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58ca61f08e62436966665ed71f74e4f56a14bd82

I will push this changes to mozilla-inbound.

Comment 52

2 years ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6203d3f7c36d
IonMonkey: Implement MathFunction(Floor) recover instruction r=nbp
https://hg.mozilla.org/mozilla-central/rev/6203d3f7c36d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.