542016 - Remove LIR_addp from jit since MMgc conservatively marks interior pointers

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

Description

[Edwin Smith]

The JIT uses LIR_addp as a workaround to ensure that when a computed interior pointer is live across a call, that its base pointer is also live.  If the GC is able to objects when it only sees an interior pointer on the stack, the JIT doesn't have to worry about it.

If the GC is permanently able to do this, we can remove LIR_addp entirely from nanojit.  But at least we can tie use of LIR_addp to the feature that controls the GC stack-scan code.

Comment 1

[Edwin Smith]

Attachment: Don't use LIR_addp if MMGC_INTERIOR_PTRS is enabled

Comment 2

[Edwin Smith]

Attachment: Removes LIR_iaddp, qaddp, and the LIR_addp alias

Comment 3

[Edwin Smith]

Attachment: Removes use of LIR_addp entirely, not based on a feature switch.

Comment 4

[Nicholas Nethercote [inactive]]

I'm more than happy to kill off these instructions (it goes very well with the addition of LIR_addxov in bug 539874).  But I don't understand how this TR code could possibly work:

#ifdef MMGC_INTERIOR_PTRS
    // use plain add, since gc doesn't need to scan base
    static const LOpcode LIR_add_ptr = LIR_piadd;
#else
    static const LOpcode LIR_add_ptr = LIR_addp;
#endif

Since LIR_addp no longer exists, and LIR_piadd never existed AFAICT.

AIUI, addp can be used when the condition codes don't need to be set, right?  But with addxov then it becomes like this:  'add' doesn't need the condition codes set;  'addxov' does, but it's internal to the code generated for the instruction.

Comment 5

[Edwin Smith]

I marked that patch obsolete and added an alternate patch for TR that just removes use of LIR_addp completely.  LIR_piadd already existed, it's an alias in LIR.h which becomes either LIR_add (32-bit) or LIR_qiadd (64-bit).

Correct, LIR_addp wasn't required to set condition codes, so switching to LIR_add was okay.  TR doesn't currently use LIR_ov, and I'm aware that with LIR_addxov coming along, the hazard of accidentally setting condition codes inbetween LIR_add and LIR_ov will go away.  in other words, this hazard goes away:

  LIR_add
  LIR_addp  (known to not set CC's, but unsafe to change to LIR_add)
  LIR_ov
  LIR_jt

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> I marked that patch obsolete and added an alternate patch for TR that just
> removes use of LIR_addp completely.

The alternative patch is identical to the obsoleted patch, I think you attached the wrong one.

Comment 7

[Nicholas Nethercote [inactive]]

I think LIR_addp is the only binary instruction that isn't CSEable.  If that's correct, then once it is removed CseFilter::ins2() can be changed from this:

  if (isCseOpcode(v)) {
      ...
      return exprs->add(LIns2, ins, k);
  }
  return out->ins2(v,a,b);

to this:

  NanoAssert(isCseOpcode(v));
  ...
  return exprs->add(LIns2, ins, k);

Comment 8

[Nicholas Nethercote [inactive]]

What's the status here?  Still interested?  (And see comment 6.)

Comment 9

[Edwin Smith]

Comment on attachment 423380 [details] [diff] [review]
Removes use of LIR_addp entirely, not based on a feature switch.

This can go into tamarin-redux soonish, if we have high confidence that MMgc support for stack/register based interior pointers is permanent and working as advertized.

Comment 10

[Lars T Hansen]

Comment on attachment 423380 [details] [diff] [review]
Removes use of LIR_addp entirely, not based on a feature switch.

To the best of our knowledge, MMgc's support for retaining objects reached via pointers into the objects from the stack and registers is stable.

In case I did not note it earlier, pointers must point /into/ the object, they cannot point /past/ the object and you should be careful with boundary cases where the code might advance a pointer past the end of an object (even just to the next word).

Comment 11

[Rick Reitmaier]

I wonder if we can dig up the tests where this failed?  IIRC Tommy might have a good test case.

Comment 12

[Edwin Smith]

(In reply to comment #10)
> (From update of attachment 423380 [details] [diff] [review])
> To the best of our knowledge, MMgc's support for retaining objects reached via
> pointers into the objects from the stack and registers is stable.
> 
> In case I did not note it earlier, pointers must point /into/ the object, they
> cannot point /past/ the object and you should be careful with boundary cases
> where the code might advance a pointer past the end of an object (even just to
> the next word).

The nanojit uses are very narrow: they just calculate the address of a field inside an object, and only when the layout of the object is known.  I'll take one more look before landing just to be sure.

The calculated interior pointer only resides in a register or stack-spill location; it is never written into the heap.

Comment 13

[Edwin Smith]

Attachment: replaces LIR_addp uses with LIR_piadd (pointer-sized add)

All but two uses of LIR_addp was pointer calculations inside stack memory allocated with LIR_alloc.  In those cases, LIR_piadd would have already been safe.

The remaining two cases are:
1. pointer to the boolean strings "true" or "false" stored in core.booleanStrings[]
2. pointer passed to the RC write barrier.

both cases point from the stack/registers into the heap allocated object, not past the start or end.

Comment 14

[Edwin Smith]

bug 418889 is the original bug that motivated the addp instruction.  Code generation has changed significantly to the point that the example code would't have caused a lingering interior pointer anyway.  (LIR supports ld [disp+ptr]).

At this point I think any test-writing effort should be aimed directly at testing MMgc's interior pointer support since the jit and C++ compilers both can generate similar constructs.  A C++ test case that explicitly makes an interior pointer, clobbers the base pointer, then forces collection will be clearer and easier to maintain, probably.

Comment 15

[Tommy Reilly]

+1  a  potential recipe:

Create a RCObject subclass sentinel who's dtor tells us a ZCT reap has occurred.   Create a sentinel.   Then create RCObject's in a loop and store interior pointers to each one in an alloca'd list and break when the ZCT reap occurs.    After breaking confirm that all the RCObjects are valid using GC::IsPointerIntoGCObject on all elements in the alloca array.

Comment 16

[Edwin Smith]

Comment on attachment 423379 [details] [diff] [review]
Removes LIR_iaddp, qaddp, and the LIR_addp alias

The second patch (for TR) must land first, and this one is slightly out of date, but it's just code removal so I thought I'd ask for review early.

Comment 17

[Edwin Smith]

Attachment: (v2) Removes LIR_iaddp, qaddp, and the LIR_addp alias

Rebased, and simplifies logic in CseFilter::ins2 according to comment #7.  Depends on the TR patch landing first.

Comment 18

[Tommy Reilly]

Comment on attachment 434093 [details] [diff] [review]
replaces LIR_addp uses with LIR_piadd (pointer-sized add)

is "pi" pointer integer?

Comment 19

[Edwin Smith]

Comment on attachment 434093 [details] [diff] [review]
replaces LIR_addp uses with LIR_piadd (pointer-sized add)

LIR_piadd is an alias to LIR_add (32bit int add) or LIR_qiadd (64bit int add), depending on platform.

the names are not symmetric but that will soon be fixed.

Comment 20

[Edwin Smith]

Comment on attachment 434093 [details] [diff] [review]
replaces LIR_addp uses with LIR_piadd (pointer-sized add)

pushed
http://hg.mozilla.org/tamarin-redux/rev/8bd955e72e28

Comment 21

[Edwin Smith]

Attachment: TM patch: replaces uses of LIR_addp with LIR_piadd

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 434531 [details] [diff] [review]
(v2) Removes LIR_iaddp, qaddp, and the LIR_addp alias

Lovely!  This will help with bu 504506, because the one overlap between the old names and proposed new names was "LIR_addp".

Comment 23

[Edwin Smith]

Comment on attachment 434531 [details] [diff] [review]
(v2) Removes LIR_iaddp, qaddp, and the LIR_addp alias

pushed NJ patch: http://hg.mozilla.org/projects/nanojit-central/rev/0f21522488fc

Comment 24

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/61a4b97b627f
http://hg.mozilla.org/tracemonkey/rev/5166cbc96bb6

Comment 25

[Edwin Smith]

This fix causes an increase in register pressure because of more effective CSE, in a situation like this:

class A {
   var v1 = function() { }
   var v2 = function() { }
   ...
   var vN = function() { }
}

The initializer function for A will be, roughly:

ld3 = .../* this, an instance of A */

// vm-generated init code
add1 = add ld3, 20
atomWriteBarrier(#core, ld3, add1, 4) /* initialize v1 to undefined */
add2 = add ld3, 24
atomWriteBarrier(#core, ld3, add2, 4) /* initialize v2 to undefined */
...
addN = add ld3, 20+4*N
atomWriteBarrier(#core, ld3, addN, 4) /* initialize v2 to undefined */

// later, ABC-generated init code
function1 = ... /* creates closure 1 */
atomWriteBarrier(#core, ld3, add1, function1) /* initialize v1 */
function2 = ... /* creates closure 2 */
atomWriteBarrier(#core, ld3, add2, function2) /* initialize v2  */
...

in other words, in the second block of N initialization expressoins, the CSE engine finds and reuses the N adds from the first block, creating register pressure for N temp values.

The old code used addp, which CSE ignored, which reduced register pressure.

This is not a bug, per se, but it caused this change in the prolog:
-......   sub sp, sp, #0x290
+......   sub sp, sp, #0x440

in other words, we went from needing 164 words of stack (0x290 = 656 bytes = 164 words) to needing 272 words, over the threshold of NJ_MAX_STACK_ENTRY = 256.

conclusion: not a bug with removing addp, but as we continue to make CSE better, we also need to make rematerialization smarter, or perhaps tweak the frontend slightly.

in this situation, changing the api for atomWriteBarrier to take pointer and an offset instead of two pointers, would eliminate the adds and reduce register pressure for long initialization sequences.

Comment 26

[Nicholas Nethercote [inactive]]

(In reply to comment #25)
> in other words, we went from needing 164 words of stack (0x290 = 656 bytes =
> 164 words) to needing 272 words, over the threshold of NJ_MAX_STACK_ENTRY =
> 256.

NJ_MAX_STACK_ENTRY is 4096 on i386, X64, PPC; 8192 on Sparc;  256 on ARM and MIPS.

Maybe the ARM and MIPS ones should be made bigger?  And I don't see why Sparc can't be 4096 as well.

Comment 27

[Edwin Smith]

NJ patch in TR: 
http://hg.mozilla.org/tamarin-redux/rev/2549da5f53a2

> Maybe the ARM and MIPS ones should be made bigger?  And I don't see why Sparc
> can't be 4096 as well.

They probably all should be big, and the same size.  will create a followup bug.

Comment 28

[Krzysztof Palacz]

Making NJ_MAX_STACK_ENTRY bigger on the ARM would (positively) affect bug 550273.

Comment 29

[Edwin Smith]

> They probably all should be big, and the same size.  will create a followup
> bug.

Bug 556175

Comment 30

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/61a4b97b627f