515309 - nanojit: kill reservations in the ARM backend

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 514349 started killing off Reservations.  This needs to continue in the ARM backend.  I can work on this, slowly, via my qemu system.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

Patch gives 1 failure on trace-tests (trace-test/tests/basiv/testEliminatedGuardWithinAnchor.js), which is the same as what I get without it.

For the review, it'll be easier if you familiarise yourself with the new Reservation-avoiding functions like isUsed(), hasKnownReg(), getReg(), which are all grouped together in LIR.h.

Comment 2

[Jacob Bramley [:jbramley]]

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

Overall, it's a good clean-up and looks good. I've just got a
couple of comments, and I also hit a build problem:

 * I don't like the assignment within the condition statement here:

    if (arg->isUsed() && isKnownReg(rr = arg->getReg())) {

   This kind of thing looks like an error (even though it isn't),
   and it tends to make the code hard to read. I see that you're
   trying to optimize the conditional statement so that
   arg->getReg() is not calculated if !arg->isUsed(), so maybe
   this is the cleanest way to do it, but there's usually a
   better way. Nesting the "if" could get ugly here, but there
   are a couple of ways around this.

   This isn't mentioned in the coding style guide, so feel free
   to override my preference.

 * You removed the intermediate callRes value around line 860,
   but there's still one left in the "else" clause, on line 874.
   (This breaks the build for me.)

   Similarly, there's some weirdness with the NanoAssert on line
   877; you've modified it to call ins->getReg() instead of just
   using rr, even though rr is used as-is in the following FMDRR
   call (and is initialized to ins->getReg() anyway).

 * We had a NanoAssert(resv) on line 1156; can we have a
   NanoAssert(ins->isUsed()) to ensure the same level of
   assertiveness? (If I've misunderstood, and ins->isUsed() and
   (bool)resv aren't completely equivalent, then please correct
   me!)

 * At line 1808, I'd prefer an "if" statement to a ternary
   operator; it's just neater.

   For example:

    Register sr;
    if (lhs->isUnusedOrHasUnknownReg()) {
        sr = findRegFor(lhs, FpRegs);
    } else {
        sr = lhs->getReg();
    }

   It's six lines of code, as opposed to three, but it's
   much more readable and it makes it easier to spot potential
   errors.

   This also occurs at lines 2060 and 2204. Indeed, it might be
   nice to make a utility function to do this, but it's not
   really important for now.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> (From update of attachment 399673 [details] [diff] [review])
> Overall, it's a good clean-up and looks good. I've just got a
> couple of comments, and I also hit a build problem:
> 
>  * I don't like the assignment within the condition statement here:
> 
>     if (arg->isUsed() && isKnownReg(rr = arg->getReg())) {

Agree on this being poor style -- if isKnownReg were a macro that evaluated its parameter more than once, trouble (it might be renamed IS_KNOWN_REG to telegraph its hazardous-macro nature, but it might not).

>  * At line 1808, I'd prefer an "if" statement to a ternary
>    operator; it's just neater.
> 
>    For example:
> 
>     Register sr;
>     if (lhs->isUnusedOrHasUnknownReg()) {
>         sr = findRegFor(lhs, FpRegs);
>     } else {
>         sr = lhs->getReg();
>     }
> 
>    It's six lines of code, as opposed to three, but it's
>    much more readable

Readability changes -- suggest you get used to ?:, we use it heavily to avoid the six lines instead of one or three, and to common the left-hand side of assignment.

> and it makes it easier to spot potential errors.

That depends entirely on reading the C idiom or the bulky if-else.

I'm with Nick on ternary >> if-else for such expressions, and so is the prevailing SpiderMonkey style.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 399673 [details] [diff] [review] [details])
> > Overall, it's a good clean-up and looks good. I've just got a
> > couple of comments, and I also hit a build problem:
> > 
> >  * I don't like the assignment within the condition statement here:
> > 
> >     if (arg->isUsed() && isKnownReg(rr = arg->getReg())) {
> 
> Agree on this being poor style -- if isKnownReg were a macro that evaluated its
> parameter more than once, trouble (it might be renamed IS_KNOWN_REG to
> telegraph its hazardous-macro nature, but it might not).

Just to dwell on this fine style point a bit more: splitting the if is ugly both because it adds extra statement and indentation overhead, and also because it can require retesting or duplicating if there's an else:

  if (A && B(x = y)) { C } else { D } =>
  if (A) { x = y; if (B(x)) { C } else { D} } else { D }

We sometimes use the comma operator, which otherwise is a crutch so macros can sequence expressions without making statements:

  if (A && (x = y, B(x))) { C } else { D }

/be

Comment 5

[Jeff Walden [:Waldo]]

If doing |fun(a = b)| and intentionally keeping it that way (comma may work fine here as Brendan notes), at the very least over-parenthesize |a = b| to make the assignment more obvious.

+    Register sr = ( lhs->isUnusedOrHasUnknownReg()
+                  ? findRegFor(lhs, FpRegs)
+                  : lhs->getReg() );

Is this weird, superfluous, extra-space parenthesization nanojit's style?  Ugh if so.  :-\

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch v2

(In reply to comment #2)
>
>  * I don't like the assignment within the condition statement here:
> 
>     if (arg->isUsed() && isKnownReg(rr = arg->getReg())) {

I avoided that one by using hasKnownReg().  That even moves the Register 
declaration inside the if-branch, reducing its scope, which is nice.

More broadly, when this situation arises (the assignment is necessary to avoid 
duplicating the else branch) do people have strong opinions about whether
over-parenthesizing or using a comma is better?  I'd lean towards a comma.
 
>  * You removed the intermediate callRes value around line 860,
>    but there's still one left in the "else" clause, on line 874.
>    (This breaks the build for me.)

Fixed this.  (It was a last-minute change, I didn't test it carefully enough, 
sorry.)
 
>    Similarly, there's some weirdness with the NanoAssert on line
>    877; you've modified it to call ins->getReg() instead of just
>    using rr, even though rr is used as-is in the following FMDRR
>    call (and is initialized to ins->getReg() anyway).

I was preserving the old behaviour here, because ins->getReg() is equivalent to 
callRes->reg.  But you're right that using 'rr' is clearer, so I've changed it.

>  * We had a NanoAssert(resv) on line 1156; can we have a
>    NanoAssert(ins->isUsed()) to ensure the same level of
>    assertiveness? (If I've misunderstood, and ins->isUsed() and
>    (bool)resv aren't completely equivalent, then please correct
>    me!

ins->regReg() and disp(ins) both assert that ins's reservation is used, so an 
assertion here is unnecessary.

(BTW, did you know you can quote the diff in the review by choosing the patch's 
"details" and then "edit attachment as comment"?  I find it easier to read 
reviews that use this feature rather than saying "on line 1156".)
 
>  * At line 1808, I'd prefer an "if" statement to a ternary
>    operator; it's just neater.

This seems like a pretty clean use of ?: to me... are there any situations 
where you'd accept using it?  Anyway, I'm happy to defer to Brendan on this one
:)

>    This also occurs at lines 2060 and 2204. Indeed, it might be
>    nice to make a utility function to do this, but it's not
>    really important for now.

Good suggestion, but slightly outside the scope of this bug, so I've added it 
as a todo for bug 515313.

I also moved a disp() call, to fix one of those 
how-on-earth-did-this-ever-work? bugs.

Thanks for the careful review!

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> 
> +    Register sr = ( lhs->isUnusedOrHasUnknownReg()
> +                  ? findRegFor(lhs, FpRegs)
> +                  : lhs->getReg() );
> 
> Is this weird, superfluous, extra-space parenthesization nanojit's style?  Ugh
> if so.  :-\

It's njn style (nanojit doesn't have a very strong style).  I particularly like it for multi-line ?: expressions because the leading '(' lines up with the '?' and ':'.  Sometimes I even put the trailing ')' on a new line.

More generally, I find the don't-ever-over-parenthesize style surprising.  As a young'un I was told "C has 15 levels of operator precedence.  You will never remember them all.  So remember that '*' and '/' bind tighter than '+' and '-', and use parentheses for everything else."  While this overstates it a little, I like this kind of defensive programming.  Each to his own, I guess.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> More broadly, when this situation arises (the assignment is necessary to avoid 
> duplicating the else branch) do people have strong opinions about whether
> over-parenthesizing or using a comma is better?  I'd lean towards a comma.

Comma.

(In reply to comment #7)
> (In reply to comment #5)
> > 
> > +    Register sr = ( lhs->isUnusedOrHasUnknownReg()
> > +                  ? findRegFor(lhs, FpRegs)
> > +                  : lhs->getReg() );
> > 
> > Is this weird, superfluous, extra-space parenthesization nanojit's style?  Ugh
> > if so.  :-\
> 
> It's njn style (nanojit doesn't have a very strong style).  I particularly like
> it for multi-line ?: expressions because the leading '(' lines up with the '?'
> and ':'.  Sometimes I even put the trailing ')' on a new line.

There are several gravity wells. If you don't over-parenthesize, the ? and : might want to go under the = operator. Or (dmr style parenthesizes conditions that aren't primary or member expression) under the left paren of the condition (not of the whole ?:).

The latter is SpiderMonkey house style, and SpiderMonkey house style eschews over-parenthesization except where precedence is botched in C and you will get a warning from gcc.

It's not clear what's best but we try to stick to one style. For nanojit it is really up to you ;-).

> More generally, I find the don't-ever-over-parenthesize style surprising.  As a
> young'un I was told "C has 15 levels of operator precedence.  You will never
> remember them all.  So remember that '*' and '/' bind tighter than '+' and '-',
> and use parentheses for everything else."  While this overstates it a little, I
> like this kind of defensive programming.

SpiderMonkey hackers have to know JS and C (now C++) so must memorize the operator precedence hierarchy, warts and all. It is an entrance exam (waived in gal's case :-P).

/be

Comment 9

[Jacob Bramley [:jbramley]]

Ooh, what a big old can of worms! I don't think this is the right place to discuss style policy so I'll resist the urge to argue for my own preferences, but you guys know Mozilla's coding styles better than I do so if it's purely a stylistic thing, please override me. (I only raised things that I haven't seen much of in NativeARM.cpp.)

The general rule of thumb that I go by is that if it really matters, it should be here: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
Otherwise, I go with either whatever is already done elsewhere in the file, or common sense.

> ins->regReg() and disp(ins) both assert that ins's reservation is used, so an 
> assertion here is unnecessary.

Oops! Yep, you're right.

> (BTW, did you know you can quote the diff in the review by choosing the patch's 
> "details" and then "edit attachment as comment"?  I find it easier to read 
> reviews that use this feature rather than saying "on line 1156".)

I had pushed the button in the past to see what it did, but didn't think to use it here until I'd already written out the review. I'll do this next time!

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> 
> The general rule of thumb that I go by is that if it really matters, it should
> be here: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
> Otherwise, I go with either whatever is already done elsewhere in the file, or
> common sense.

But Nanojit is its own entity, and doesn't have any style guide (at least, not that I know of, and it's a bit of a mishmash).

Comment 11

[Brendan Eich [:brendan]]

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style
https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style

FWIW (these are already out of date a bit, but not far; the Mozilla Coding Style Guide was updated recently, but conflicts on a few points with the JS style).

/be

Comment 12

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/271317c77463

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8827216e53b2