Closed Bug 1104647 Opened 10 years ago Closed 7 years ago

IonMonkey: Implement MathFunction(Floor) recover instruction

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

Product:
Core
Component:
JavaScript Engine: JIT
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

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

References

(Blocks 1 open bug)

Details

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

Attachments

(6 files, 1 obsolete file)

rev 1 - Implement MathFunction(Floor)
895 bytes, patch
Details | Diff | Splinter Review
rev 2 - Implement MathFunction(Floor) recover instruction
1.55 KB, patch
nbp
: feedback+
Details | Diff | Splinter Review
rev 2 - floor number (func 152 pass 16)
233.17 KB, image/png
Details
rev 2 - floor object (func 153 pass 16)
322.08 KB, image/png
Details
ianschweer_progress.patch
604 bytes, patch
Details | Diff | Splinter Review
floor-no-sse4.patch
2.46 KB, patch
nbp
: review+
Details | Diff | Splinter Review 
Implement MathFunction(Floor) in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

This issue is similar to Bug 1028573.
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 ?!
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 -
        Flags: review?(nicolas.b.pierron)

    
    

    
  


  

        Attachment #8537717 -
        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)

    
    

    
  
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+

    
  

        Attachment #8537715 -
        Flags: review?(nicolas.b.pierron)

    
    

    
  
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)

    
    

    
      
  

    
    

    
  
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!

    
    

    
  
(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

    
    

    
  
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)

    
    

    
  
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

    
    

    
  
(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?

    
    

    
  
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

    
    

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

    
    

    
  
(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

    
    

    
  
Awesome, thanks for the help Nicolas!

Could someone assign this ticket to me?

Ian

    
  
Assignee: nobody → schweerian33

    
    

    
  


  

    
    

    
  
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

    
    

    
  
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)

    
    

    
  
Hey Nicolas,

Can you assist me in writing a test case? 

Ian

    
    

    
  
(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?

    
    

    
  
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

    
    

    
  
(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 ?

    
    

    
  
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

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        math-floor.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8925372 -
        Flags: review?(nicolas.b.pierron)

    
    

    
  
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

    
    

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

    
    

    
  
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+

    
    

    
  


  

        Attachment #8925372 -
        Attachment is obsolete: true

        Attachment #8926284 -
        Flags: review?(nicolas.b.pierron)

    
    

    
  
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?

    
    

    
  
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+

    
    

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

    
    

    
  
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.

    
    

    
  
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
Closed: 7 years ago

          status-firefox58:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: