Closed Bug 1127932 Opened 5 years ago Closed 5 years ago

IonMonkey: Inline SIMD.float32x4.add calls

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: nbp, Assigned: bjdevel50, Mentored)

References

Details

Attachments

(1 file, 2 obsolete files)

The goal of this bug is to inline SIMD.float32x4.and calls from js/src/jit-tests/tests/asm.js/simd-mandelbrot.js benchmarks when the JavaScript shell is ran with --no-asmjs command line argument.

To solve this issue, you have modify, build and check the JavaScript shell [1].
Then identify if the optimization is working well by running the jit-test as follow:

  $ jit-test/jit_test.py --ion -s -o path/to/js asm.js/simd-mandelbrot.js

To display the command line, then prefix the command line as describe in the documentation for using iongraph [2], and use iongraph tool to see if the last MIR phase is changed and if the SIMD.float32x4.and operation got inlined with a MSimdBinaryArith.

You can refer to Bug 1118344 for one example.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_%28JS_shell%29
Hi, could I work on this bug? The description seems clear enough to implement the change.
Jarda
(In reply to Jarda from comment #1)
> Hi, could I work on this bug? The description seems clear enough to
> implement the change.
> Jarda

Sure, as you already made a patch before, I am assigning you on this bug ;)
Assignee: nobody → bjdevel50
Fixing a typo …

> The goal of this bug is to inline SIMD.float32x4.*add* calls
Summary: IonMonkey: Inline SIMD.float32x4.and calls → IonMonkey: Inline SIMD.float32x4.add calls
Attached patch simd.float32x4add (obsolete) — Splinter Review
Hi, I would need an advice on how to actually do it. I thought it would be simple, but my patch produces a core dump. Running it with gdb does not give any hints as it suggests the stack is corrupted. Valgrind also does not give any hint. Setting IONFLAGS=bl-all suggests the crash happens during inlining, but do not know at which stage. 
Could you, please, suggest what could be wrong?
Attachment #8559050 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8559050 [details] [diff] [review]
simd.float32x4add

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

The patch looks good, but I have no idea what might be wrong, does iongraph output suggest any bad MIRType? There is a bailout, do we know the reason of the bailout?  You can try to add breakpoint in the generated code [1], especially around CodeGenerator::visitSimdBox / CodeGenerator::visitSimdUnbox.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Setting_a_breakpoint_in_the_generated_code_%28from_gdb.2C_x86_.2F_x86-64.2C_arm%29

::: js/src/jit/BaselineIC.cpp
@@ +9106,5 @@
> +                return false;
> +            return true;
> +        }
> +
> +        if (native == js::simd_float32x4_add && JitSupportsSimd()) {

nit: remove JitSupportsSimd() condition.
Attachment #8559050 - Flags: review?(nicolas.b.pierron)
Hey, the recent alignment work seems to have fixed this issue (tested locally for you)! Please fix the nit of the previous comment and feel free to re-upload a patch, ask nbp again for review.  Thanks for your contribution!
Status: NEW → ASSIGNED
Flags: needinfo?(bjdevel50)
I think we can extend the scope of this bug to inlining float32x4.add/sub/mul, as it's pretty trivial to do, thanks to Victor's patches :)
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Hey, the recent alignment work seems to have fixed this issue (tested
> locally for you)! Please fix the nit of the previous comment and feel free
> to re-upload a patch, ask nbp again for review.  Thanks for your
> contribution!

I am glad the problem is solved. Could you point me to the bug, where the alignment was treated? I am curious what was happening.

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> I think we can extend the scope of this bug to inlining
> float32x4.add/sub/mul, as it's pretty trivial to do, thanks to Victor's
> patches :)

OK. This seems straightforward. I post a patch soon.
Flags: needinfo?(bjdevel50)
Attached patch simd.float32x4.add.sub.mul (obsolete) — Splinter Review
As suggested, the patch also supports mul/sub operations on float32x4. As the code to inline function call of arithmetic operations on int32x4 is very similar to float32x4, it was made more general by adding the simd type as a parameter.
Attachment #8559050 - Attachment is obsolete: true
Attachment #8567804 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8567804 [details] [diff] [review]
simd.float32x4.add.sub.mul

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

This patch sounds good to me, but as Benjamin has been working a lot on this area lately, and he has conflicting patches, I will let him do the review ;)
Attachment #8567804 - Flags: review?(nicolas.b.pierron)
Attachment #8567804 - Flags: review?(benj)
Attachment #8567804 - Flags: feedback+
Comment on attachment 8567804 [details] [diff] [review]
simd.float32x4.add.sub.mul

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

Nice! This patch will need rebasing over the patches I've landed this morning, I will do it on your behalf and land your patch then. Thanks for doing it!
Attachment #8567804 - Flags: review?(benj) → review+
Attached patch addsubmul.patchSplinter Review
This is the rebased patch, plus tests in it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c9bb91285d
Attachment #8567804 - Attachment is obsolete: true
Attachment #8568467 - Flags: review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> Created attachment 8568467 [details] [diff] [review]
> addsubmul.patch
> 
> This is the rebased patch, plus tests in it.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c9bb91285d

Nice, there does not seem to be much left from my patch :-)
(In reply to Jarda from comment #13)
> Nice, there does not seem to be much left from my patch :-)

No, as the ideas you applied were the same I've applied in the patch which landed yesterday morning :)
Another try run, just to make sure these red herrings are due to another changeset.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1774188641ba
If we take the green union of the two try runs, we get a fully green try run. Let's land it!
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=02cbdac24ec2
https://hg.mozilla.org/mozilla-central/rev/02cbdac24ec2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.