903394 - IonMonkey: Remove do {} while(false) in IonBuilder

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

I think it is very ugly to have do {} while(false) in the code. But that is a personal opinion. Now for getprop we used getPropTry... helper functions. We can do the same for setprop/setelem.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug903394-setelem-remove-do-while

1) I didn't fold jsop_setelem_dense and jsop_setelem_typed into the setElemTryDense/setElemTryTyped, since the function are both quite big...

2) Jandem, feel free to forward the review to somebody if you approve to use of "setElemTryXXX" instead of do_while_false. It is pure refactor, no new functionality at all.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: bug903394-setprop-remove-do-while

Comment 3

[Hannes Verschore [:h4writer]]

I can do the same for getelem for consistency too (no do-while in there).

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 788108 [details] [diff] [review]
bug903394-setelem-remove-do-while

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

::: js/src/jit/IonBuilder.cpp
@@ +7057,5 @@
> +{
> +    JS_ASSERT(*emitted == false);
> +
> +    if (object->type() != MIRType_Magic)
> +        return false;

Oops, should be: "return true;"

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 788108 [details] [diff] [review]
bug903394-setelem-remove-do-while

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

\o/ Nice refactoring!

::: js/src/jit/IonBuilder.cpp
@@ +7073,5 @@
> +    if (!object->mightBeType(MIRType_Object))
> +        return true;
> +
> +    if (!index->mightBeType(MIRType_Int32) &&
> +            !index->mightBeType(MIRType_String))

Style nit:

    if (!index->mightBeType(MIRType_Int32) && !index->mightBeType(MIRType_String))
        return true;

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 788109 [details] [diff] [review]
bug903394-setprop-remove-do-while

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

Very nice.

::: js/src/jit/IonBuilder.cpp
@@ +8340,5 @@
> +    // the setter's return value.
> +    CallInfo callInfo(cx, false);
> +    if (!callInfo.init(current, 1))
> +        return false;
> +    // Ensure that we know we are calling a setter in case we inline it.

Pre-existing nit: add a newline before this comment.

@@ +8382,5 @@
> +    // Emit SetDOMProperty.
> +    JS_ASSERT(setter->jitInfo()->type == JSJitInfo::Setter);
> +    MSetDOMProperty *set = MSetDOMProperty::New(setter->jitInfo()->setter, obj, value);
> +    if (!set)
> +        return false;

Pre-existing nit: can remove this NULL check; we don't have them for other MIR instructions.

@@ +8448,5 @@
> +
> +    if (shapes.length() == 1) {
> +        spew("Inlining monomorphic SETPROP");
> +
> +        // The JM IC was monomorphic, so we inline the property access as

Pre-existing nit: s/JM/Baseline

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: bug903394-getelem-remove-do-while WIP

Decided to do getelem too. Failing jit-tests atm, probably just a typo, but not awake enough to find it.

Comment 8

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f324fcd43ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5a7b55c22a

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9f324fcd43ec
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5a7b55c22a

One of the patch in the range of [1] is causing deterministic failures while running octane-richards on Unagi.  Hannes any idea, or I should back out them?

[1] http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=928f0878d1cdef204feaed4e66a8f4baadf09d8f&tochange=6a5a7b55c22a954f12f4c7e8e178706daa806fe7

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> One of the patch in the range of [1] is causing deterministic failures while
> running octane-richards on Unagi.

Not richards, but delta-blue.  I still get a result displayed on richards and the crash happens asafter the delta blue box appears on the screen.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 788108 [details] [diff] [review]
bug903394-setelem-remove-do-while

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

::: js/src/jit/IonBuilder.cpp
@@ +6971,5 @@
> +
> +    TypedArrayObject *tarr = &tarrObj->as<TypedArrayObject>();
> +    ArrayBufferView::ViewType viewType = JS_GetArrayBufferViewType(tarr);
> +
> +    MDefinition *ptr = convertShiftToMaskForStaticTypedArray(index, viewType);

This function should be splitted in 2 as it add MDefinitions to the MIR Graph and it might still return NULL if it cannot.  This is confusing.

@@ +7056,5 @@
> +                                MDefinition *index, MDefinition *value)
> +{
> +    JS_ASSERT(*emitted == false);
> +
> +    if (object->type() != MIRType_Magic)

You also need to guard against MIRType_Value which might be a not definitively lazy argument.

It was written like that in the original code.

     if (script()->argumentsHasVarBinding() && object->mightBeType(MIRType_Magic))
         return abort("Type is not definitely lazy arguments.");

So I guess we need a setElemTryMaybeArguments(…) which is testing that and returns with an abort.

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f815c1dcf93f - https://tbpl.mozilla.org/php/getParsedLog.php?id=26381631&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=26381604&tree=Mozilla-Inbound, mochitest-3 and dromaeo crashing in js::types::StackTypeSet::propertyNeedsBarrier.

Comment 13

[Hannes Verschore [:h4writer]]

Bug 904024 is actually why this was backed out. We don't have type information on the returned object from PropertyWriteNeedsTypeBarrier. Since this is only refactor, I'll reland by letting it behave like before the refactor. I'll improve the situation in the attached bug.

Comment 14

[Hannes Verschore [:h4writer]]

Try push to be sure the Mochitests/dromaeo issues are fixed:
https://tbpl.mozilla.org/?tree=Try&rev=cf2ffdd1f9fa

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> Comment on attachment 788108 [details] [diff] [review]
> bug903394-setelem-remove-do-while
> 
> Review of attachment 788108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +6971,5 @@
> > +
> > +    TypedArrayObject *tarr = &tarrObj->as<TypedArrayObject>();
> > +    ArrayBufferView::ViewType viewType = JS_GetArrayBufferViewType(tarr);
> > +
> > +    MDefinition *ptr = convertShiftToMaskForStaticTypedArray(index, viewType);
> 
> This function should be splitted in 2 as it add MDefinitions to the MIR
> Graph and it might still return NULL if it cannot.  This is confusing.

I think we do that all over the place. So I don't see why this should be wrong...

> 
> @@ +7056,5 @@
> > +                                MDefinition *index, MDefinition *value)
> > +{
> > +    JS_ASSERT(*emitted == false);
> > +
> > +    if (object->type() != MIRType_Magic)
> 
> You also need to guard against MIRType_Value which might be a not
> definitively lazy argument.
> 
> It was written like that in the original code.
> 
>      if (script()->argumentsHasVarBinding() &&
> object->mightBeType(MIRType_Magic))
>          return abort("Type is not definitely lazy arguments.");
> 
> So I guess we need a setElemTryMaybeArguments(…) which is testing that and
> returns with an abort.

This test is still present in the refactored code. I don't see a reason to add the "setElemTryMaybeArguments" function for this, yet.

Comment 15

[Hannes Verschore [:h4writer]]

Attachment: bug903394-getelem-remove-do-while

I was indeed very close. This passes jit-tests. Started already a try build so I don't have a backout with this patch:

https://tbpl.mozilla.org/?tree=Try&rev=901d02accfbf

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #14)
> Try push to be sure the Mochitests/dromaeo issues are fixed:
> https://tbpl.mozilla.org/?tree=Try&rev=cf2ffdd1f9fa
> 
> (In reply to Nicolas B. Pierron [:nbp] from comment #11)
> > Comment on attachment 788108 [details] [diff] [review]
> > bug903394-setelem-remove-do-while
> > 
> > Review of attachment 788108 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/IonBuilder.cpp
> > @@ +6971,5 @@
> > > +
> > > +    TypedArrayObject *tarr = &tarrObj->as<TypedArrayObject>();
> > > +    ArrayBufferView::ViewType viewType = JS_GetArrayBufferViewType(tarr);
> > > +
> > > +    MDefinition *ptr = convertShiftToMaskForStaticTypedArray(index, viewType);
> > 
> > This function should be splitted in 2 as it add MDefinitions to the MIR
> > Graph and it might still return NULL if it cannot.  This is confusing.
> 
> I think we do that all over the place. So I don't see why this should be
> wrong...

Because we do not see the pivot between not-emitted and start-emitting code.

> > 
> > @@ +7056,5 @@
> > > +                                MDefinition *index, MDefinition *value)
> > > +{
> > > +    JS_ASSERT(*emitted == false);
> > > +
> > > +    if (object->type() != MIRType_Magic)
> > 
> > You also need to guard against MIRType_Value which might be a not
> > definitively lazy argument.
> > 
> > It was written like that in the original code.
> > 
> >      if (script()->argumentsHasVarBinding() &&
> > object->mightBeType(MIRType_Magic))
> >          return abort("Type is not definitely lazy arguments.");
> > 
> > So I guess we need a setElemTryMaybeArguments(…) which is testing that and
> > returns with an abort.
> 
> This test is still present in the refactored code. I don't see a reason to
> add the "setElemTryMaybeArguments" function for this, yet.

Sorry, I did not see that it remained as-is, as I expected it to be moved too.

Comment 17

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7842d98c6d32
https://hg.mozilla.org/integration/mozilla-inbound/rev/debfceea4af3

Try push was green, re-pushing.

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 788919 [details] [diff] [review]
bug903394-getelem-remove-do-while

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

::: js/src/jit/IonBuilder.cpp
@@ +6641,5 @@
> +
> +    if (obj->type() != MIRType_String)
> +        return true;
> +
> +    // Emit index to a string.

Nit: something like "// Emit fast path for string[index]."

::: js/src/jit/IonBuilder.h
@@ +398,5 @@
>                           MDefinition *index, MDefinition *value);
>  
> +    // jsop_getelem() helpers.
> +    bool getElemTryDense(bool *emitted, MDefinition *obj,
> +                         MDefinition *index);

Style nit: all these declarations fit on one line I think, same for the .cpp file probably.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7842d98c6d32
https://hg.mozilla.org/mozilla-central/rev/debfceea4af3

Comment 20

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/003dff773a2a

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/003dff773a2a