723636 - IonMonkey: implement MRegExp as a warmup op

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: MRegExp without TI-based native optimizations

Starting to get my benchmark on.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Whoops, left a masm.breakpoint comment in there. Will remove that before commit.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 593913 [details] [diff] [review]
MRegExp without TI-based native optimizations

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

Looks good, r=me with the comments below addressed.

::: js/src/ion/CodeGenerator.cpp
@@ +261,5 @@
> +    static const VMFunction js_CloneRegExpObjectInfo = FunctionInfo<pf>(js_CloneRegExpObject);
> +
> +    //masm.breakpoint();
> +    pushArg(ImmWord(lir->mir()->source()));
> +    pushArg(ImmWord(proto));

Use ImmGCPtr instead of ImmWord when baking in pointers to GC things, so that they can be relocated. And remove the masm.breakpoint() comment while you're here.

::: js/src/ion/IonBuilder.cpp
@@ +3411,5 @@
> +    MRegExp *ins = MRegExp::New(reobj, MRegExp::MustClone);
> +    current->add(ins);
> +    current->push(ins);
> +
> +    return true;

I don't think we want to execute this instruction twice when we bailout (see also the AliasSet comment below), so you'll need something like

if (ins->isEffectful() && !resumeAfter(ins))
    return false;

::: js/src/ion/MIR.h
@@ +2033,5 @@
> +        MustClone
> +    };
> +
> +  private:
> +    RegExpObject *source_;

I think this should be HeapPtr<RegExpObject> nowadays, see also MNewArray::type_.

@@ +2062,5 @@
> +    CloneBehavior shouldClone() const {
> +        return shouldClone_;
> +    }
> +    AliasSet getAliasSet() const {
> +        return AliasSet::None();

This means the instruction is not effectful, so for instance DCE will remove MRegExp if it has no uses and we may execute this JSOP_REGEXP twice if an instruction following it bails out. I'm not sure if that's okay.

It's probably best to use "all side-effects" for now (if shouldClone == MustClone), but we should discuss this with dvander and sstangl.

::: js/src/ion/x86/CodeGenerator-x86.h
@@ +101,5 @@
>      bool visitWriteBarrierT(LWriteBarrierT *barrier);
>      bool visitLoadElementT(LLoadElementT *load);
>      bool visitStoreElementV(LStoreElementV *store);
>      bool visitStoreElementT(LStoreElementT *store);
> +    bool visitMRegExp(LRegExp *lir);

You can remove this line.

::: js/src/jsinterp.cpp
@@ +886,5 @@
>              *equal = true;
>              return true;
>          }
> +        JS_ASSERT_IF(lref.isBoolean(), lref.isTrue() || lref.isFalse());
> +        JS_ASSERT_IF(rref.isBoolean(), rref.isTrue() || rref.isFalse());

Nit: remove these asserts?

::: js/src/jsscriptinlines.h
@@ +168,5 @@
>      JSObjectArray *arr = regexps();
>      JS_ASSERT(uint32_t(index) < arr->length);
>      JSObject *obj = arr->vector[index];
>      JS_ASSERT(obj->isRegExp());
> +    return (js::RegExpObject *) obj;

Nit: Can we replace the last two lines with obj->asRegExp()?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij (:jandem) from comment #2)
> 
> And remove the masm.breakpoint() comment while you're here.

Ignore this line, I missed comment 1.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Jan de Mooij (:jandem) from comment #2)
> Use ImmGCPtr instead of ImmWord when baking in pointers to GC things, so
> that they can be relocated.

Okay, I'm guessing this is future-proofing for a time when we persist code across moving GCs? It was my understanding we could generally burn in pointers under the assumption code would get thrown out on each GC.

> I don't think we want to execute this instruction twice when we bailout

It's fine if we do, we just create an additional RegExp object. We generally assume bailouts to be rare, right?

> I think this should be HeapPtr<RegExpObject> nowadays, see also
> MNewArray::type_.

Okay, more future proofing? Sounds good.

> @@ +2062,5 @@
> > +    CloneBehavior shouldClone() const {
> > +        return shouldClone_;
> > +    }
> > +    AliasSet getAliasSet() const {
> > +        return AliasSet::None();
> 
> This means the instruction is not effectful, so for instance DCE will remove
> MRegExp if it has no uses and we may execute this JSOP_REGEXP twice if an
> instruction following it bails out. I'm not sure if that's okay.

Yeah, that's ok.
 
> It's probably best to use "all side-effects" for now (if shouldClone ==
> MustClone), but we should discuss this with dvander and sstangl.

I don't understand?

> ::: js/src/jsinterp.cpp
> @@ +886,5 @@
> >              *equal = true;
> >              return true;
> >          }
> > +        JS_ASSERT_IF(lref.isBoolean(), lref.isTrue() || lref.isFalse());
> > +        JS_ASSERT_IF(rref.isBoolean(), rref.isTrue() || rref.isFalse());
> 
> Nit: remove these asserts?

How about I move them to isTrue / isFalse? Nice to detect corrupted boolean values with an assert rather than a strange assertEq failure like I got (where the type was appropriately boolean but it was neither true nor false).

> ::: js/src/jsscriptinlines.h
> @@ +168,5 @@
> >      JSObjectArray *arr = regexps();
> >      JS_ASSERT(uint32_t(index) < arr->length);
> >      JSObject *obj = arr->vector[index];
> >      JS_ASSERT(obj->isRegExp());
> > +    return (js::RegExpObject *) obj;
> 
> Nit: Can we replace the last two lines with obj->asRegExp()?

I would have liked to, but it seems to require a circular js*inlines include to avoid the "annotated inline but no inline definition is available" warning in GCC, so I just duplicated it there. :-(

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij (:jandem) from comment #2)
> @@ +2062,5 @@
> > +    CloneBehavior shouldClone() const {
> > +        return shouldClone_;
> > +    }
> > +    AliasSet getAliasSet() const {
> > +        return AliasSet::None();
> 
> This means the instruction is not effectful, so for instance DCE will remove
> MRegExp if it has no uses and we may execute this JSOP_REGEXP twice if an
> instruction following it bails out. I'm not sure if that's okay.
> 
> It's probably best to use "all side-effects" for now (if shouldClone ==
> MustClone), but we should discuss this with dvander and sstangl.

LICM would be needed on MRegExp for the first loop of string-validate-input.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Hey David, would you mind weighing in on the "is a potentially GCing operation effectful" question?  I don't really buy the argument in bug 685838 comment 21, just because our LIR ops are all "atomically" interacting with the GC in a sense -- you'll never reorder a LIR op between two "halves" of a GC operation.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Potentially GC operations aren't effectful, but for operations which are expensive or allocate a lot of memory, we might not want to execute them twice. For example, a NEWARRAY with 50,000 slots can be repeated, but we probably don't want to repeat it.

I don't know how expensive JSOP_REGEXP is. If it's just allocating a JSObject, that doesnt seem too bad. If it's compiling a regular expression too, maybe not.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to David Anderson [:dvander] from comment #7)
> I don't know how expensive JSOP_REGEXP is. If it's just allocating a
> JSObject, that doesnt seem too bad. If it's compiling a regular expression
> too, maybe not.

The latter! Agreed on the many-slot concern, but this one should be a drop in the bucket. Thanks for the feedback!

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Chris Leary [:cdleary] from comment #8)
> (In reply to David Anderson [:dvander] from comment #7)
> > I don't know how expensive JSOP_REGEXP is. If it's just allocating a
> > JSObject, that doesnt seem too bad. If it's compiling a regular expression
> > too, maybe not.

Er, the middle-r. No compilation.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/projects/ionmonkey/rev/f6b54f8e87b2

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Looks like this may have regressed testMatchStringObject.js and/or testLet.js -- not sure why I didn't see those locally. I'll back out and investigate.

https://hg.mozilla.org/projects/ionmonkey/rev/f7f49506bdc9 (backout)

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Blah, args were being pushed first-to-last instead of last-to-first.

https://hg.mozilla.org/projects/ionmonkey/rev/902afe4fb2fa