565251 - TM: simplify TraceRecorder::guardClass()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

TraceRecorder::guardClass() is a strange function:

  JS_REQUIRES_STACK bool
  TraceRecorder::guardClass(JSObject* obj, LIns* obj_ins, JSClass* clasp, VMSideExit* exit,
                            AccSet accSet)
  {
      bool cond = obj->getClass() == clasp;

      LIns* class_ins = lir->insLoad(LIR_ldp, obj_ins, offsetof(JSObject, classword), accSet);
      class_ins = lir->ins2(LIR_piand, class_ins, INS_CONSTWORD(~JSSLOT_CLASS_MASK_BITS));

      guard(cond, addName(lir->ins2(LIR_peq, class_ins, INS_CONSTPTR(clasp)), namebuf), exit);
      return cond;
  }

What's strange is the 'cond'.  We pass in a JSObject and a JSClass*.  If
the JSObject has the given class, we generate "exit if not that class" code.
Otherwise we generate "exit if that class code".  And then we return 'cond'.

In other words, guardClass() combines a test with some LIR generation (and
the LIR generation is dependent on the result of the test).  This is confusing.

Furthermore, a lot of the time we know from the calling context that the 
JSObject has the given class, because we have either this:

  guardClass(obj, obj_ins, obj->getClass(), ...);

Or something equivalent to this:

  if (obj->hasClass(clasp))
    guardClass(obj, obj_ins, clasp, ...);

Also, sometimes we abort recording if guardClass() returns false, in which
case we could have just checked the clasp and aborted without generating the
LIR.

This patch splits guardClass() into guardClass() and guardNotClass(),
removes the no-longer-needed 'obj' argument, removes the return value, and
adjusts all the call sites so any tests are done before
guardClass()/guardNotClass() is called.  This makes the calls easier to
understand.  It also makes guard more amenable to optimization -- the proposed optimization in bug 565250 is much easier with this patch.

Comment 1

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

Comment on attachment 444828 [details] [diff] [review]
patch

Nice simplification.

>         // Make sure the array is actually dense.
>-        if (!guardDenseArray(obj, obj_ins, BRANCH_EXIT))
>+        if (obj->isDenseArray()) 
>+            guardDenseArray(obj_ins, BRANCH_EXIT);
>+        else
>             return ARECORD_STOP;

Better to invert the test and drop the else:

> if (!obj->isDenseArray())
>     return ARECORD_STOP;
> guardDenseArray(obj_ins, BRANCH_EXIT);

r=me with that

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> (From update of attachment 444828 [details] [diff] [review])
> Nice simplification.

Yes, thanks njn! The early TM days had some guardClass evolution which, IIRC, introduced cond and the two-way is/is-not behavior.

> >         // Make sure the array is actually dense.
> >-        if (!guardDenseArray(obj, obj_ins, BRANCH_EXIT))
> >+        if (obj->isDenseArray()) 
> >+            guardDenseArray(obj_ins, BRANCH_EXIT);
> >+        else
> >             return ARECORD_STOP;
> 
> Better to invert the test and drop the else:
> 
> > if (!obj->isDenseArray())
> >     return ARECORD_STOP;
> > guardDenseArray(obj_ins, BRANCH_EXIT);

Right -- otherwise the non-aborting path A;B;C is oddly split: if (A) B; else return R; C. B is overindented as well as not adjacent to C, also losing. This is a SpiderMoneky style guide item, IIRC.

/be

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch v2

Fixes the nits from the review, and uses guardClass() in one more place where it's appropriate, within unbox_jsval().

Comment 4

[Nicholas Nethercote [inactive]]

Sorry, the interdiff fails because of opcode renaming that happened between v1 and v2.

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/11db2881b9f3

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/11db2881b9f3