1012632 - IonMonkey: Implement Mod Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Implement RMod in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

Comment 1

[Amol Mundayoor]

Attachment: b1012632.patch

Comment 2

[Amol Mundayoor]

Ni Nicolas,

Once I figured this out, I kind of feel like an idiot for taking so long :).

I would like help with building the IonGraph. I cloned the github repository and moved it to m-c/js/src/build-dbg/. Within this directory, I wanted to ran the command:

IONFILTER=pdfjs.js:16934 IONFLAGS=logs,scripts,osi,bailouts ./dist/bin/js ./run.js 2>&1 | less

Problem is that run.js is not in this directory. Where should I run the command?

In the meantime, I have uploaded a patch with all my work so far.

Comment 3

[Amol Mundayoor]

I also have worries about the quality of the test, and whether a print statement within tests would not be pertinent. Thank you for all the help, I appreciate it.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Amol Mundayoor from comment #2)
> I would like help with building the IonGraph. I cloned the github repository
> and moved it to m-c/js/src/build-dbg/. Within this directory, I wanted to
> ran the command:
> 
> IONFILTER=pdfjs.js:16934 IONFLAGS=logs,scripts,osi,bailouts ./dist/bin/js
> ./run.js 2>&1 | less

In the previous command line "./dist/bin/js" is the JS VM, which is used to run "./run.js".  Then during the execution we filter some verbose mode with "IONFLAGS=help" and only if we are compiling the function located in "pdfjs.js:16934".  I guess you will have to adapt this to your test case, such as you execute "dce-wit-rinstructions.js" instead of "./run.js" which is an example taken from the octane benchmark.

(In reply to Amol Mundayoor from comment #3)
> I also have worries about the quality of the test, and whether a print
> statement within tests would not be pertinent. Thank you for all the help, I
> appreciate it.

The print is less pertinent than assertEq, as we are not checking the output (only the error output).
It might be easier to debug, but this is useless for the test case.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8430572 [details] [diff] [review]
b1012632.patch

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

You are almost there.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +15,5 @@
> +var uceFault_mod_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_mod_number'));
> +function rmod_number(i) {
> +	var x = i % 11;
> +	if (uceFault_mod_number(i) || uceFault_mod_number(i))
> +		print(x);

Use assertEq, instead of print.

@@ +21,5 @@
> +}
> +
> +var uceFault_mod_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_mod_float'));
> +function rmod_float(i) {
> +	var x = i % 11.11;

Mod cannot be specialized with Float (instead of Double), in which case we do not distinguish them from Numbers as they are computed identically in the recover function.

@@ +29,5 @@
> +}
> +
> +var uceFault_mod_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_mod_object'));
> +function rmod_object(i) {
> +	var x = {} + i;

See new examples of *_object tests.

::: js/src/jit/Recover.cpp
@@ +228,5 @@
> +MMod::writeRecoverData(CompactBufferWriter &writer) const
> +{
> +	MOZ_ASSERT(canRecoverOnBailout());
> +	writer.writeUnsigned(uint32_t(RInstruction::Recover_Mod));
> +	writer.writeByte(specialization_ == MIRType_Float32);

Does this case ever happen ?!

::: js/src/jit/Recover.h
@@ +135,5 @@
> +class RMod MOZ_FINAL : public RInstruction
> +{
> +  private:
> +    bool isFloatOperation_;
> +    

nit: remove trailing white-spaces.

Comment 6

[Amol Mundayoor]

Attachment: b1012632.patch

I've made the changes requested. 

I'm still struggling with the IONGRAPH - when I run 

IONFLAGS=logs,bailouts ../dist/bin/js ../../jit-test/tests/ion/dce-with-rinstructions.js 

I get errors saying "Can't log script dce-with-instructions.js:1 (Compiled on background thread)". What does this mean?

When I run make for ion graph (wasn't expecting it to work), I get errors saying "Cannot access *.gv: No such file or directory". I have installed graphviz using apt-get. Am I missing any dependencies? Or is it because it couldn't log the script? 

Thanks

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Amol Mundayoor from comment #6)
> I'm still struggling with the IONGRAPH - when I run 
> 
> IONFLAGS=logs,bailouts ../dist/bin/js
> ../../jit-test/tests/ion/dce-with-rinstructions.js 
> 
> I get errors saying "Can't log script dce-with-instructions.js:1 (Compiled
> on background thread)". What does this mean?

We should improve this error message, if you want to do so after, feel free ;)
The reason you have this message is that we are compiling the script out-side the main thread, while we are running code.  As these compilations might interleaved with other things, we need to serialize everything to prevent the interleaving of messages.

So, just add the argument --ion-parallel-compile=off to the JavaScript shell ;)

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8431777 [details] [diff] [review]
b1012632.patch

Sorry I am a bit busy right now.
I will try to review your patch by Monday evening ;)

Comment 9

[Amol Mundayoor]

Thanks for the update Nicolas.

I re-ran it with ion-parallel-compile=off and I get

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Bad decoding of the previous instruction?), at /home/am/dev/spidermonkey/js/src/jit/Recover.cpp:50

This looks like it was triggered from an invalid recovery. How can I debug this issue? Thanks.

Comment 10

[Amol Mundayoor]

Oh yeah and I'll gladly fix that error message. Incomprehensible outputs like that just leave me confused - and I don't like being confused ;).

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Amol Mundayoor from comment #9)
> Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Bad decoding of the
> previous instruction?), at
> /home/am/dev/spidermonkey/js/src/jit/Recover.cpp:50
> 
> This looks like it was triggered from an invalid recovery. How can I debug
> this issue? Thanks.

use "gdb --args " in front of your command line, and remove the environment variable as it should not be needed to reproduce this assertion.  Then use gdb to investigate what is wrong.

This error message indicates that one of the recover instructions might have consume either too many or not enough bytes compared to how it is encoded.  Which cause us to interpret some data which does not correspond to a valid identifier of a valid recover instruction.

Comment 12

[Amol Mundayoor]

Attachment: b1012632.patch.latest

I fixed the bug. IONGRAPHS were generated successfully. A lot of PNGs and PDFs were generated, ~600 with the following command:

IONFLAGS=logs,bailouts ./dist/bin/js --ion-parallel-compile=off ../jit-test/tests/ion/dce-with-rinstructions.js 

I've updated the patch with the latest version, and marked the rest obsolete. I would like to know how I can give you the output of the IONGRAPH. Which one is the right one? How do I know? Do I need to re-run the command with different arguments?

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Amol Mundayoor from comment #12)
> I've updated the patch with the latest version, and marked the rest
> obsolete. I would like to know how I can give you the output of the
> IONGRAPH. Which one is the right one? How do I know? Do I need to re-run the
> command with different arguments?

Look at the latest compilation of the function in which you are interested in.  Recover instructions are introduced during the Dead Code Elimination phase.  If the recover instruction is expected to work it should appear in gray, like the resume points.

Comment 14

[Amol Mundayoor]

Attachment: iongraph_number.png

Attaching ion graph of mod_number

Comment 15

[Amol Mundayoor]

Attachment: iongraph_object.png

Attaching ion graph of object

Comment 16

[Nicolas B. Pierron [:nbp]]

This looks good :)
Notice that the MMod of an object is not flagged as being recovered on bailout, as expected.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8431959 [details] [diff] [review]
b1012632.patch.latest

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

Still some clean-up to do, and this would be ready to test and land in Firefox code base ;)
Also, do not forget to ask for review next time, set the review flag to "?" and write ":nbp".

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +15,5 @@
> +var uceFault_mod_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_mod_number'));
> +function rmod_number(i) {
> +	var x = i % 98;
> +	if (uceFault_mod_number(i) || uceFault_mod_number(i))
> +		assertEq(x, 1); // 99 % 11 = 0 

nit: update the comment and remove the trailing white space.

@@ +23,5 @@
> +var uceFault_mod_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_mod_object'));
> +function rmod_object(i) {
> +	var t = i;
> +	var o = { valueOf: function() { return t; } };
> +	var x = o % i;

You can keep the % 98 as above, this would simplify the understand of the test ;)

::: js/src/jit/Recover.cpp
@@ +247,5 @@
> +		return false;
> +
> +	iter.storeInstructionResult(result);
> +	return true;
> +}	

style-nit: remove the trailing white space after the curly braces.

::: js/src/jit/Recover.h
@@ +134,5 @@
>  
> +class RMod MOZ_FINAL : public RInstruction
> +{
> +  private:
> +    bool isFloatOperation_;

This field is no longer used, remove it ;)

@@ +145,5 @@
> +    }
> +    
> +    bool recover(JSContext *cx, SnapshotIterator &iter) const;
> +};
> +	

style-nit: Remove all trailing white-space from the 4 blank lines above.

Comment 18

[Amol Mundayoor]

Attachment: b1012632.patch

Updated patch to remove all trailing white spaces.

Thank you for the note about requesting review. I did it this time :).

Comment 19

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8432042 [details] [diff] [review]
b1012632.patch

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

Ok, one last test case fix, and we are good to go ;)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +26,5 @@
> +	var o = { valueOf: function() { return t; } };
> +	var x = o % 98;
> +	t = 1000;
> +	if(uceFault_mod_object(i) || uceFault_mod_object(i))
> +		assertEq(x,0) /* modding a number with itself equals zero */;

I would expect this test case to fail?  I am expecting an issue in the test case ;)

::: js/src/jit/Recover.h
@@ +142,5 @@
> +    }
> +
> +    bool recover(JSContext *cx, SnapshotIterator &iter) const;
> +};
> +	

style-nit: last trailing-whitespace survivor after the end of the structure ;)

Comment 20

[Amol Mundayoor]

Attachment: b1012632.patch

You're correct, that test case fails. I have edited it in this patch. How did you know? Is it because it is essentially the same test from the value perspective but from the type perspective it is not (Object vs Number) and I was expecting different results (0 vs 1)?

Also, is it a bad test if the results for all subtests is the same? For instance both the number and object computation ends up with the same result. Also, does MDN have documentation on how to write good tests? I'm very new to unit testing :).

Let's hope that trailing whitespace is gone for ever - I've updated this report with the latest patch :).

Comment 21

[Amol Mundayoor]

Attachment: b1012632.patch

Oops accidentally uploaded an old one

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Amol Mundayoor from comment #20)
> You're correct, that test case fails. I have edited it in this patch. How
> did you know? Is it because it is essentially the same test from the value
> perspective but from the type perspective it is not (Object vs Number) and I
> was expecting different results (0 vs 1)?

Mostly yes, and I also know how to emulate a modulo operator as well as a small expressions of JavaScript in my head ;)

> Also, is it a bad test if the results for all subtests is the same? For
> instance both the number and object computation ends up with the same
> result.

This would be a bad test if we were testing Modulo.  Our goal is not to prove that our implementation of modulo is correct, as the code use a simple function call and this function is well tested already.

Our goal is to check that the recover instruction is correctly executed, and that it does not introduce bugs, which might appear as a different behavior in the evaluation of modulo.  So the test case is exhaustive in these terms.

> Also, does MDN have documentation on how to write good tests? I'm
> very new to unit testing :).

I am not aware of any.

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8432860 [details] [diff] [review]
b1012632.patch

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

This sounds good, I will push the change to our Try-server and if it pass, I will push your change to mozilla-inbound ;)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +25,5 @@
> +	var o = { valueOf: function() { return t; } };
> +	var x = o % 98;
> +	t = 1000;
> +	if(uceFault_mod_object(i) || uceFault_mod_object(i))
> +		assertEq(x,1) /* modding a number with itself equals zero */;

nit: I will update the comment while pushing ;)

Comment 24

[Nicolas B. Pierron [:nbp]]

Link to our try-server, where I ran the check of all jit-tests:
  https://tbpl.mozilla.org/?tree=Try&rev=2349d3c0e71f

Comment 25

[Nicolas B. Pierron [:nbp]]

Congratulation :)

I landed[2] the patch in Firefox 32, you can follow how your patch is going on our contiguous integration infrastructure[1].

[1] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2bd26c86bbd0
[2] https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd26c86bbd0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd26c86bbd0

Comment 27

[Amol Mundayoor]

Thank you all! I'm super happy for having contributed and I'd like to continue :).

In Comment #7, Nicolas mentioned that I could fix up an error message if I felt like it. I do feel like it, and found that the message is located at Ion.cpp:1914. Are there any preferences on what this should change to? I was simply thinking of appending "Re-run with flag --ion-parallel-compile=off" or something similar to the error string.