500053 - TM: encapsulate LIns construction into LIns class

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

LirBufWriter no longer needs to be friends with LIns, and we can assembly Lir manually without LirBufWriter (which is what I am really after).

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

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

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

>-        l->initOpcodeAndClearResv(LIR_skip);
>-        l->setOprnd1((LInsp)prevLInsAddr);
>+		l->setIns1(LIR_skip, (LInsp)prevLInsAddr);

Do you need to also clear the resv there?

Comment 3

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/1b2ecfa68c38

Comment 4

[Nicholas Nethercote [inactive]]

This will completely screw my variable-width LIR patch (bug 492866).

Can you explain more the benefit of unfriending LIns and LirBufWriter?

Comment 5

[Nicholas Nethercote [inactive]]

This change will have broken the Sparc back-end.

Comment 6

[Andreas Gal :gal]

#4: I am not opposed to friending the two classes, I just removed it since its no longer necessary. We can add it back.

#5: can you explain?

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
> #4: I am not opposed to friending the two classes, I just removed it since its
> no longer necessary. We can add it back.

In the end I worked around it for variable-width LIR.  I'll be posting an updated patch to bug 492866 soon.

> #5: can you explain?

Sorry, I should have given more detail:  there's an i->clearResv() call that should be changed to i->resv()->clear().  So just a small thing.

Comment 8

[Andreas Gal :gal]

#4: we should land the patch soon. the constant re-basing must be unpleasant

#5: ARM too I guess. Want me to patch this or did you already do it?

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #8)
> 
> #5: ARM too I guess. Want me to patch this or did you already do it?

There's a call to clearResv() in NativeArm.cpp but it's commented out.  Might be worth patching anyway, I'm happy for you to do it.

Comment 10

[Andreas Gal :gal]

Ok, I will file a follow-up bug and fix sparc and ARM.

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1b2ecfa68c38