Open Bug 998485 Opened 6 years ago Updated 9 months ago

Replace EdgeCaseAnalysis (Forward & Backward), by equivalent RangeAnalysis collectRangeInfoPreTrunc(ate)

Categories

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

defect

Tracking

()

People

(Reporter: nbp, Unassigned, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file)

Currently we have a few optimizations which are edge case analysis[1].  This optimizations is nice, but now that we have a better Range Analysis[2], we are capable of describing the range of any operation, and not only constants.

We should move the logic of analyzeEdgeCasesForward() and analyzeEdgeCasesBackward() to the the RangeAnalysis.cpp file which contains equivalent functions which are named collectRangeInfoPreTrunc().

Equalities, inequalities can be translated into Range's methods[3], such as we can use these predicates instead on the range of operations instead of checking only on the value of a constant.

The basic principle behind the Range Analysis is that we annotate operations as such

  if (x >= 10) {
    // x range's is [10, +inf]
    if (x <= 20) {
      // x range's is [10, 20]
      y = x + 5;
      // y range's is [15, 25]
    }
  }

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#1500
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#2580
[3] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.h#455
Depends on: 1023300
Depends on: 1025050
Depends on: 1026365
Depends on: 1026512
Mentor: nicolas.b.pierron, sunfish
Whiteboard: [mentor=nbp,sunfish][lang=c++] → [lang=c++]
I think this can get closed?
(In reply to Hannes Verschore [:h4writer] from comment #1)
> I think this can get closed?

As far as I know, the code for analyzeEdge* functions should be removed too.
I want to work on this bug.
Assignee: nobody → contact
Attached patch bug998485.patchSplinter Review
Attachment #8454694 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8454694 [details] [diff] [review]
bug998485.patch

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

This looks good, but does this impact performance?
Can you check against sunspider, kraken and octane as we do on arewefastyet.com ?

Note that you can use an optimized shell build for testing these benchmarks instead of building a browser.
I have just run sunspider, kraken and octane on my computer. Should I upload here the results ?
(In reply to Rémi WENG from comment #6)
> I have just run sunspider, kraken and octane on my computer. Should I upload
> here the results ?

if there any slow-down which is not accounted by the noise of the benchmarks, compared to the JS Shell without this patch?
(In reply to Nicolas B. Pierron [:nbp] {N/A 22-29/08} from comment #7)
> (In reply to Rémi WENG from comment #6)
> > I have just run sunspider, kraken and octane on my computer. Should I upload
> > here the results ?
> 
> if there any slow-down which is not accounted by the noise of the
> benchmarks, compared to the JS Shell without this patch?

I have run these tests only twice (once with this patch and another without) and I checked the results "by hand". But, I does not see a very big impact on the performance.
Comment on attachment 8454694 [details] [diff] [review]
bug998485.patch

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

After removing the reference of EdgeCaseAnalysis.cpp from the js/src/moz.build file, this patch compiles fine.

remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=a2f372232028
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a2f372232028

::: js/src/jit/MIR.cpp
@@ -1542,5 @@
> -
> -void
> -MDiv::analyzeEdgeCasesBackward()
> -{
> -    if (canBeNegativeZero() && !NeedNegativeZeroCheck(this))

Note: NeedNegativeZeroCheck is no longer used after this patch.  I removed after locally in the same patch.
Attachment #8454694 - Flags: review?(nicolas.b.pierron) → review+
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/43ad7f4f490e

This patch caused a 14% regression on Octane GameBoy benchmark:
http://www.arewefastyet.com/#machine=11&view=single&suite=octane&subtest=Gameboy

Rémi, can you investigate why we have this regression, and find a fix for it?
Ok, I will found out a solution.
nbp, maybe we can mentor another contributor?
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P5
(In reply to Jan de Mooij [:jandem] from comment #13)
> nbp, maybe we can mentor another contributor?

Yes, definitely.
Assignee: contact → nobody
Flags: needinfo?(nicolas.b.pierron)
Hi, I would like to work on this but I need mentoring. Can someone advice me on the first steps? Where can I find the documentations for the relevant code as I can't find them on the Mozilla website and the commenting in the source code is pretty sparse. Thanks.
Flags: needinfo?(nicolas.b.pierron)
Hi,

(In reply to chinhanglun from comment #15)
> Hi, I would like to work on this but I need mentoring. Can someone advice me
> on the first steps?

Here are a few steps to continue fixing this issue:
 - Build SpiderMonkey JS shell. [1]
 - Include the current patch from this bug, build another SpiderMonkey shell in order to compare with the previous build.
 - Run octane benchmark and compare it with the non-patched version. [2]
 - Possibly look at iongraph differences between the non-patched version and the patched version. [3]
 - Try to fix the reason which makes us behave differently.

If you have any specific question, feel free to ask here, or on #jsapi (irc.mozilla.org) [4]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[2] https://github.com/h4writer/arewefastyet/tree/master/benchmarks/octane
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_(JS_shell)
[4] https://wiki.mozilla.org/IRC
Flags: needinfo?(nicolas.b.pierron)
Mentor: sunfish
You need to log in before you can comment on or make changes to this bug.