Closed
Bug 1104647
Opened 10 years ago
Closed 7 years ago
IonMonkey: Implement MathFunction(Floor) recover instruction
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla58
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)
895 bytes,
patch
|
Details | Diff | Splinter Review | |
1.55 KB,
patch
|
nbp
:
feedback+
|
Details | Diff | Splinter Review |
233.17 KB,
image/png
|
Details | |
322.08 KB,
image/png
|
Details | |
604 bytes,
patch
|
Details | Diff | Splinter Review | |
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.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
Attachment #8536211 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 11•10 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)
Comment 12•10 years ago
|
||
Attachment #8537712 -
Flags: review?(nicolas.b.pierron)
Comment 13•10 years ago
|
||
Attachment #8537715 -
Flags: review?(nicolas.b.pierron)
Comment 14•10 years ago
|
||
Attachment #8537717 -
Flags: review?(nicolas.b.pierron)
Updated•10 years ago
|
Attachment #8537715 -
Attachment description: rev 2 - floor number (func 153 pass 16) → rev 2 - floor number (func 152 pass 16)
Reporter | ||
Comment 15•10 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•10 years ago
|
Attachment #8537715 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 16•10 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) |
Comment 18•9 years ago
|
||
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•9 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.
Comment 20•8 years ago
|
||
Dongie, are you still interested in working on this?
Flags: needinfo?(dongie.agnir)
Priority: -- → P5
Assignee | ||
Comment 21•8 years ago
|
||
Hello Till, Is it possible to work on this? Is someone else assigned? Ian
Flags: needinfo?(till)
Comment 22•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Awesome, thanks for the help Nicolas! Could someone assign this ticket to me? Ian
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → schweerian33
Assignee | ||
Comment 29•8 years ago
|
||
Assignee | ||
Comment 30•8 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•8 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•8 years ago
|
||
Hey Nicolas, Can you assist me in writing a test case? Ian
Reporter | ||
Comment 33•8 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•8 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•8 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•8 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•8 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•8 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•7 years ago
|
||
Is this still an open bug? Can i pick it up?
Comment 40•7 years ago
|
||
Are you still working on it? Or can Malik take over?
Flags: needinfo?(schweerian33)
Assignee | ||
Comment 41•7 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
Comment 42•7 years ago
|
||
I'll take this one. I've built Spidermonkey, and I'm going over the explanation detailed in Bug 1003801
Comment 43•7 years ago
|
||
Attachment #8925372 -
Flags: review?(nicolas.b.pierron)
Comment 44•7 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•7 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•7 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•7 years ago
|
||
Attachment #8925372 -
Attachment is obsolete: true
Attachment #8926284 -
Flags: review?(nicolas.b.pierron)
Comment 48•7 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•7 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•7 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•7 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•7 years ago
|
||
Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6203d3f7c36d IonMonkey: Implement MathFunction(Floor) recover instruction r=nbp
Comment 53•7 years ago
|
||
bugherder |
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.
Description
•