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 ;)

Fixing a typo … > The goal of this bug is to inline SIMD.float32x4.*add* calls

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?

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.

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

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.

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 ;)

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!

This is the rebased patch, plus tests in it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c9bb91285d

(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