1066041 - IonMonkey: Implement TypeOf recover instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[Nicolas B. Pierron [:nbp]]

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

Comment 2

[Connor [:cojo]]

Attachment: 1066041-typeof-impl.diff

I have not worked on a javascript bug before so I'm not sure if the implementation is correct for RTypeOf::recover. I'm also not sure if the tests are coded correctly, I just tried my best to emulate what other folks have done.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8489028 [details] [diff] [review]
1066041-typeof-impl.diff

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

Welcome
This patch is awesome!

Still they are a lot of trailing white-spaces in the patch and the test cases are not checking the code which is being added. (see below)

Also, do not forget to ask for review (drop down "?" and ":nbp"), when you attach a patch, such as the reviewer get a notification.  This would prevent you from waiting too long for feedback / review ;)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +912,5 @@
>  }
>  
> +var uceFault_typeof_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_typeof_object'))
> +function rtypeof_object(i) {
> +    var x = typeof {};

This code will not trigger any recover instruction, if you look at the iongraph output[1], you will find that GVN  remove the TypeOf instruction and replace it by a constant string which correspond to "object".

The only way to prevent this is to make sure the Jit cnnot infere the result of the TypeOf, such as:


    var inputs = [ {}, []. 1. true, Symbol(), undefined, function(){}, null ];
    var x = typeof (inputs[i % inputs.length]);


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_%28JS_shell%29

::: js/src/jit/Recover.cpp
@@ +975,5 @@
> +{
> +    RootedValue v(cx, iter.read());
> +    
> +    JSType type = js::TypeOfValue(v);
> +    RootedString string(cx, TypeName(type, cx->names()));

Any reason for not using TypeOfOperation? You can find the JSRuntime on all JSContext with ."cx->runtime()"

Comment 4

[Connor [:cojo]]

Attachment: 1066041-typeof-impl-2.diff

Awesome, thanks!

> This code will not trigger any recover instruction, 
> if you look at the iongraph output[1], 
> you will find that GVN remove the TypeOf instruction and 
> replace it by a constant string which correspond to "object".

I did run iongraph on the output. It ended up generating a lot of files and I wasn't sure which file was the correct file. And, to be honest, still trying to figure out how to read the output. Do you have any suggestions for resources about learning how to make sense of iongraph output?

> var inputs = [ {}, []. 1. true, Symbol(), undefined, function(){}, null ];
> var x = typeof (inputs[i % inputs.length]);

I extended your recommendation for how to trigger the code and implemented it in the condensed rtypeof function.

> Any reason for not using TypeOfOperation? 
> You can find the JSRuntime on all JSContext with ."cx->runtime()"

I missed that when I was looking for examples. When searching through dxr, I ended up finding [1] and figured it seemed like the best approach. Thanks for pointing that out, looks like it makes more sense to use.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter-inl.h#461

I've uploaded a new patch with changes. One thing I wasn't sure about:
> Still they are a lot of trailing white-spaces in the patch

Which trailing spaces are you referring to?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Connor [:cojo] from comment #4)
> Created attachment 8489616 [details] [diff] [review]
> 1066041-typeof-impl-2.diff
> 
> Awesome, thanks!
> 
> > This code will not trigger any recover instruction, 
> > if you look at the iongraph output[1], 
> > you will find that GVN remove the TypeOf instruction and 
> > replace it by a constant string which correspond to "object".
> 
> I did run iongraph on the output. It ended up generating a lot of files and
> I wasn't sure which file was the correct file. And, to be honest, still
> trying to figure out how to read the output. Do you have any suggestions for
> resources about learning how to make sense of iongraph output?

I suggest you first run IONFLAGS=scripts ./js … , then copy the location of the function in which you are interested, then run IONFILTER=<function location>  IONFLAGS=logs ./js …

This will reduce the number of files which are produced.  Then file is named func??-pass??-*.png, the one of interest is among the last one, which is Dead Code Elimination.  After this phase the "Typeof" instruction should appear in gray.

> I've uploaded a new patch with changes. One thing I wasn't sure about:
> > Still they are a lot of trailing white-spaces in the patch
> 
> Which trailing spaces are you referring to?

I was referring to what we can see with the [review] link of the previous attachment:
  https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1066041&attachment=8489028

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8489616 [details] [diff] [review]
1066041-typeof-impl-2.diff

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

Congratulation,
This patch looks good :)

I will remove the last 2 trailing white spaces and push it to our try server.  The try server a continuous integration server used for verifying patches.

Comment 7

[Nicolas B. Pierron [:nbp]]

These are the link to the try server push (we are migrating to a new UI):
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cb73b580e150

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b85cd5885e12

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b85cd5885e12