TM uses Nanojit's LIR creation API directly -- all those lir->insFoo(...) calls. I propose adding a thin layer over the top of this API to encapsulate a bunch of TM-specific things. Likely benefits: - Conciseness: For example, this: LIns* nested = lir->insBranch(LIR_jt, lir->ins2ImmI(LIR_eqi, lir->insLoad(LIR_ldi, lr, offsetof(VMSideExit, exitType), ACCSET_OTHER), NESTED_EXIT), NULL); can become something like this: LIns* nested = w.jt(w.eqiN(w.ldiVMSideExitField(lr, exitType), NESTED_EXIT) Ie. there'll be a combination of more concise names (eg. jt() vs. insBranch(LIR_jt)), and also higher-level, TM-specific functions (eg. ldiVMSideExitField()). - Correctness: This layer will be able to encapsulate the AccSets associated with loads and stores, making it much harder to get them wrong, which is important. - Demonolithization: This layer will be in a separate file, and a lot of LIR generation functions will be able to be moved out of TraceRecorder into a new class. And the LIR write pipeline should be able to be encapsulated into this class as well. I've attached a partial patch that shows the direction it's heading in. I like it a lot.

I like it. I guess some people like this, but I find it hard to read: > LIns* nested = > lir->insBranch(LIR_jt, > lir->ins2ImmI(LIR_eqi, > lir->insLoad(LIR_ldi, lr, > offsetof(VMSideExit, exitType), > ACCSET_OTHER), > NESTED_EXIT), > NULL); Much better: > LIns* nested = w.jt(w.eqiN(w.ldiVMSideExitField(lr, exitType), NESTED_EXIT)

(Luke, sorry I'm asking you to review this large patch... that's your punishment for having a reputation for good taste when it comes to code :) This patch: - Adds a tracejit/ directory, into which jstracer.cpp will eventually be broken up. - Adds tracejit/Writer.h, which contains a new class 'Writer' which implements the new LIR writing layer. - Moves the definition of the TM-specific AccSets out of jsbuiltins.h into tracejit/Writer.h. - Moves the AccSet checking from jstracer.cpp to tracejit/Writer.cpp. - Modifies jstracer.cpp to use 'Writer'. Specifically: - In TraceRecorder, the LirWriter object is gone, replaced with a Writer. Code like this: lir->ins2(LIR_addi, x, y) now looks like this: w.addi(x, y) This makes complex code generation much easier to read. jstracer.cpp *cannot* use the NJ interface any more, so there's no possibility of mixing interfaces. - The LirWriter pipeline construction is now within Writer. - There are a number of more complex LIR generating functions in Writer, eg. getStringChar(). Some functions that remain in TraceRecorder are arguably suitable for moving into Writer, but the current selection seems good enough for now. - All the INS_* macros were moved into Writer.h and function-ified where possible. - Moves demote/isPromote/etc into Writer.h. - Moves Funcfilter into Writer.h. - MemLocn objects replace base/offset/accSet trios everywhere. MemLocns are deliberately passed by value everywhere, this was simplest. Cachegrind confirms instruction counts are barely changed. - AccSets are now barely mentioned in jstracer.cpp, which is good. - Removes all the SOFTFLOAT code in jstracer.cpp. This was already slated to happen in bug 576186, it turned out to be convenient to do it in this patch. Overall, jstracer.cpp is about 900 lines shorter than it was before. I'd recommend reading Writer.h first, especially the "goals" comment at the top of class Writer. I've confirmed that the LIR generated for all of SunSpider is identical before and after the patch is applied, for both 32-bit and 64-bit platforms, which is a pretty good test. There'll be a follow-up bug to improve the naming of values in LIR dumps.

This patch is the same as the previous one but has the added bonus of compiling successfully on Windows. It passes all debug tests on the try server.

Sorry for the delay. This looks like its going to be a gratifying read :)

Rebased. Also removes LC_TMRegexp, because it's dead.

Comment on attachment 484643 [details] [diff] [review] patch, v2 (against TM 55695:0bb000635fbd) I only skimmed jstracer.cpp, but I really liked seeing everything getting cleaner. Also, on a less superficial note, I think this is also a big step for reducing potential for error. First, a high-level question: for symmetry with the method-jit, can we put Writer and everything else you decompose jstracer into in namespace js { namespace tjit { } } Another advantage is that it would avoid any dancing to avoid using the same identifier for concepts that show up in both. In fact, with two namespaces, we could actually plan to reuse the same name for the same concept and avoid "now was it Address or MemLocn"-type questions. (Granted, I don't see this happening a whole lot right now, but if the tracer continues on this path you have started, I can see a growth in WebKit-/mjit-style helper types. >+ * The Initial Developer of the Original Code is >+ * Brendan Eich <brendan@mozilla.org> I was told to put "the Mozilla Corporation" here, not to take anything from Brendan ;-) >+ w.stStateField(w.addp(w.ldpStateField(rp), >+ w.i2p(w.lshiN(w.ldiVMSideExitField(lr, calldepth), >+ sizeof(void*) == 4 ? 2 : 3))), >+ rpAtLastTreeCall); fix indent >+struct MemLocn >+{ >+ friend class Writer; >+ >+ private: IMHO, MemLocn isn't the most attractive abbreviation. For mjit symmetry (of the type mentioned at the top of this comment) you could use Address and, for the same char cost have a whole word :) >+template <class T> >+static T& >+InitConst(const T &t) >+{ >+ return const_cast<T &>(t); >+} This could be hoisted to jstl.h. >+bool isPromoteInt(nj::LIns *ins); >+bool isPromoteUint(nj::LIns *ins); >+bool isPromote(nj::LIns *ins); >+nj::LIns *demote_(nj::LirWriter *out, nj::LIns *ins); I know its pre-existing, but since you are cleaning up, could you capitalize the first letter like other non-member functions? >+ /* This is only used to call demote_(). */ >+ nj::LIns *apply(nj::LIns *(*f)(nj::LirWriter *out, nj::LIns *ins), nj::LIns *ins) const { >+ return f(lir, ins); >+ } I'm probably missing a complication, but why can't Writer::demote() just call demote_() directly? >+ Writer(nj::Allocator *alloc_, nj::LirBuffer *lirbuf_) >+ : alloc(alloc_), lirbuf(lirbuf_), lir(NULL), cse(NULL), logc(NULL) {} Usually, we don't decorate member variable names. The exception (as seen in JSStackFrame and other tiny classes) is when a canonical member function name collides. In this case, we tend to suffix the member variable with _. This code does it the other way around which I think will collide if the style spreads. >+ nj::LIns *paramp(int32 arg, int32 kind) const { >+ return lir->insParam(arg, kind); >+ } Every time I see these I have to remind myself of what arg vs. kind means. Perhaps you could take this opportunity to improve this and give the writer a more literate enumeration parameter instead? >+ /* Specific loads and stores (those not taking a MemLocn argument). Ordered by AccSets.*/ >+ >+ nj::LIns *ldStateFieldHelper(nj::LOpcode op, nj::LIns *state, int32 offset) const { >+ return lir->insLoad(op, state, offset, ACCSET_STATE); >+ } I really like what you've done with this family of memfuns! >+ bool jt(nj::LIns *cond, nj::LIns **brOut) { >+ if (cond->isImmI(1)) >+ return false; /* branch is always taken */ >+ *brOut = lir->insBranch(nj::LIR_jt, cond, NULL); /* returns NULL if never taken */ >+ return true; >+ } Awesome place for an abstraction. Could I propose the following alternative interface: // outside Writer class MaybeJump { LIns *j_ins; ... public: typedef void *ConvertibleToBool; operator ConvertibleToBool() { return js_ins; } }; // inside Writer MaybeJump jt(nj::LIns *cond); void label(MaybeJump j); This lets you write: if (MaybeJump j = w.jt(cond)) { ... w.label(j); ... } Then, you can drop the somewhat unsightly: >+ nj::LIns *jfUnoptimizable(nj::LIns *cond) const { >+ nj::LIns *jtUnoptimizable(nj::LIns *cond) const { since, with the declared MaybeJump type, there is local syntactic evidence that the code is assuming the branch isn't being optimized away. >+ /* >+ * immp() and nameImmp() can be used to embed arbitrary pointers into the >+ * native code. They should not be used directly to embed GC thing >+ * pointers. Instead, use the TraceRecorder::immpXyzGC() variants which >+ * ensure that the embedded pointer will be kept alive across GCs. >+ * >+ */ >+ nj::LIns *immp(const void *p) const { >+ return lir->insImmP(p); >+ } I think we could bring this comment into the type system a la: // outside Writer struct AlreadyRootedOrNotGCThingPtr { void *p }; struct NotGCThingPtr : AlreadyRootedOrNotGCThingPtr { /* ctor */ }; struct AlreadyRootedGCThing : AlreadyRootedOrNotGCThingPtr { /* ctor */ }; // inside Writer nj::LIns *immp(AlreadyRootedOrNotGCThingPtr p) const; So that anyone who wants to insert a GC-thing will have a loud screaming contradiction to go through. And, of course, the TraceRecorder helpers like w_immpObjGC mean that good code never has to type "AlreadyRootedGCThing". >+ * - Functions that insert moderately complex LIR sequences (eg. multiple >+ * loads) have a 'get' prefix in their name. Although I don't feel strongly, 'get' sits a little weird with me. Perhaps 'emit' as the prefix for such cases? >+ /* MemLocns, ordered by AccSet. */ >+ >+ MemLocn StackLocn(nj::LIns *base, int32 offset) { >+ return MemLocn(base, offset, ACCSET_STACK); >+ } These memfuns all seemed to be capitalized. Could they be lowercased?

(In reply to comment #6) > > >+bool isPromoteInt(nj::LIns *ins); > >+bool isPromoteUint(nj::LIns *ins); > >+bool isPromote(nj::LIns *ins); > >+nj::LIns *demote_(nj::LirWriter *out, nj::LIns *ins); > > I know its pre-existing, but since you are cleaning up, could you capitalize > the first letter like other non-member functions? Is that the convention? Seems like there are lots of existing counter-examples, but I'm happy to change. > >+ /* This is only used to call demote_(). */ > >+ nj::LIns *apply(nj::LIns *(*f)(nj::LirWriter *out, nj::LIns *ins), nj::LIns *ins) const { > >+ return f(lir, ins); > >+ } > > I'm probably missing a complication, but why can't Writer::demote() just call > demote_() directly? I was about to explain the subtlety, but it appears I've outsmarted myself and apply() isn't necessary. I'll remove it. > >+ Writer(nj::Allocator *alloc_, nj::LirBuffer *lirbuf_) > >+ : alloc(alloc_), lirbuf(lirbuf_), lir(NULL), cse(NULL), logc(NULL) {} > > Usually, we don't decorate member variable names. The exception (as seen in > JSStackFrame and other tiny classes) is when a canonical member function name > collides. In this case, we tend to suffix the member variable with _. This > code does it the other way around which I think will collide if the style > spreads. I have this vain hope of turning on -Wshadow one day, but that doesn't allow the "foo(foo)" style. Hence this style. But I can remove the trailing underscores. > >+ nj::LIns *paramp(int32 arg, int32 kind) const { > >+ return lir->insParam(arg, kind); > >+ } > > Every time I see these I have to remind myself of what arg vs. kind means. > Perhaps you could take this opportunity to improve this and give the writer a > more literate enumeration parameter instead? Yes, it's awful, but beyond the scope of this bug. Bug 545569 has a proposal to get rid of one of the two kinds. > >+ bool jt(nj::LIns *cond, nj::LIns **brOut) { > >+ if (cond->isImmI(1)) > >+ return false; /* branch is always taken */ > >+ *brOut = lir->insBranch(nj::LIR_jt, cond, NULL); /* returns NULL if never taken */ > >+ return true; > >+ } > > Awesome place for an abstraction. Could I propose the following alternative > interface: > > // outside Writer > class MaybeJump { > LIns *j_ins; > ... > public: > typedef void *ConvertibleToBool; > operator ConvertibleToBool() { return js_ins; } I don't understand what this code does -- can you explain? What is 'operator ConvertibleToBool > }; > > // inside Writer > MaybeJump jt(nj::LIns *cond); > void label(MaybeJump j); > > This lets you write: > > if (MaybeJump j = w.jt(cond)) { > ... > w.label(j); > ... > } > > Then, you can drop the somewhat unsightly: > > >+ nj::LIns *jfUnoptimizable(nj::LIns *cond) const { > >+ nj::LIns *jtUnoptimizable(nj::LIns *cond) const { > > since, with the declared MaybeJump type, there is local syntactic evidence that > the code is assuming the branch isn't being optimized away. Hmm, I like the fact that it's unsightly... if you want the short-cut version you have to clearly ask for it. Your suggestion feels a little to clever for its own good to me :/ > >+ /* > >+ * immp() and nameImmp() can be used to embed arbitrary pointers into the > >+ * native code. They should not be used directly to embed GC thing > >+ * pointers. Instead, use the TraceRecorder::immpXyzGC() variants which > >+ * ensure that the embedded pointer will be kept alive across GCs. > >+ * > >+ */ > >+ nj::LIns *immp(const void *p) const { > >+ return lir->insImmP(p); > >+ } > > I think we could bring this comment into the type system a la: > > // outside Writer > struct AlreadyRootedOrNotGCThingPtr { void *p }; > struct NotGCThingPtr : AlreadyRootedOrNotGCThingPtr { /* ctor */ }; > struct AlreadyRootedGCThing : AlreadyRootedOrNotGCThingPtr { /* ctor */ }; > > // inside Writer > nj::LIns *immp(AlreadyRootedOrNotGCThingPtr p) const; > > So that anyone who wants to insert a GC-thing will have a loud screaming > contradiction to go through. > And, of course, the TraceRecorder helpers like w_immpObjGC mean that good code > never has to type "AlreadyRootedGCThing". Hmm, in tableswitch() we currently have this: w.st(diff, w.AnyLocn(w.immp(&si->index))); Would that have to change to this? w.st(diff, w.AnyLocn(w.immp(NotGCThingPtr(&si->index)))); > >+ * - Functions that insert moderately complex LIR sequences (eg. multiple > >+ * loads) have a 'get' prefix in their name. > > Although I don't feel strongly, 'get' sits a little weird with me. Perhaps > 'emit' as the prefix for such cases? But, for example, getStringLength() emits code that, well, gets the string length. Which is similar to how, for example, ldi() emits code that does a load. "emit" doesn't match, IMHO. All the other suggestions seem fine, I'll post a new version of the patch with them tomorrow.

(In reply to comment #7) > Is that the convention? Seems like there are lots of existing > counter-examples, but I'm happy to change. Yes. I couldn't find a SM-C++-guide entry, but I've seen this SM-wide, although I wouldn't be surprised if you've seen a lot of deviation in the shadow-lands of the tracer. > > if (MaybeJump j = w.jt(cond)) { > > ... > > w.label(j); > > ... > > } > > > > Then, you can drop the somewhat unsightly: > > > > >+ nj::LIns *jfUnoptimizable(nj::LIns *cond) const { > > >+ nj::LIns *jtUnoptimizable(nj::LIns *cond) const { > > > > since, with the declared MaybeJump type, there is local syntactic evidence that > > the code is assuming the branch isn't being optimized away. > > Hmm, I like the fact that it's unsightly... if you want the short-cut version > you have to clearly ask for it. Your suggestion feels a little to clever for > its own good to me :/ It's not clever; its becoming rather common now. Perhaps you've seen: if (HashSet<T>::Ptr p = hm.lookup(k)) { ... *p } ? Same deal. The "convertible to bool" bit was just getting into minutiae too early. In general, I like "maybe" types (and I think others do too, I'm starting to see them around) vs. custom contracts b/c it bundles the conditional nature of the datum with the datum, instead, e.g. of keeping the two separate and having the caller know that they should check the return value before assuming something about the datum. > Hmm, in tableswitch() we currently have this: > > w.st(diff, w.AnyLocn(w.immp(&si->index))); > > Would that have to change to this? > > w.st(diff, w.AnyLocn(w.immp(NotGCThingPtr(&si->index)))); Yes. The verbosity doesn't seem like a bad thing; the code is doing something careful and potentially dangerous: embedding a pointer to some thing in the code and assuming the referent stays alive. Alternative names welcome -- perhaps StablePtr? > But, for example, getStringLength() emits code that, well, gets the string > length. Which is similar to how, for example, ldi() emits code that does a > load. "emit" doesn't match, IMHO. Makes sense: there is an implicit "emit" in front of every Wrapper member, so "emit emit" would be redundant :)

(In reply to comment #6) > > First, a high-level question: for symmetry with the method-jit, can we put > Writer and everything else you decompose jstracer into in > namespace js { namespace tjit { } } Done. > >+ * The Initial Developer of the Original Code is > >+ * Brendan Eich <brendan@mozilla.org> > > I was told to put "the Mozilla Corporation" here, not to take anything from > Brendan ;-) Done. > >+ w.stStateField(w.addp(w.ldpStateField(rp), > >+ w.i2p(w.lshiN(w.ldiVMSideExitField(lr, calldepth), > >+ sizeof(void*) == 4 ? 2 : 3))), > >+ rpAtLastTreeCall); > > fix indent Done. > >+struct MemLocn > >+{ > >+ friend class Writer; > >+ > >+ private: > > IMHO, MemLocn isn't the most attractive abbreviation. For mjit symmetry (of > the type mentioned at the top of this comment) you could use Address and, for > the same char cost have a whole word :) Done. I also changed StackAddress et al to sub-types instead of functions within class Writer, as per our IRC discussion. Much better. > >+template <class T> > >+static T& > >+InitConst(const T &t) > >+{ > >+ return const_cast<T &>(t); > >+} > > This could be hoisted to jstl.h. Done. I put it in the js::tl namespace, I hope this is ok. (It was unclear to me why part of that file was in js::tl and part was only in js.) > >+bool isPromoteInt(nj::LIns *ins); > >+bool isPromoteUint(nj::LIns *ins); > >+bool isPromote(nj::LIns *ins); > >+nj::LIns *demote_(nj::LirWriter *out, nj::LIns *ins); > > I know its pre-existing, but since you are cleaning up, could you capitalize > the first letter like other non-member functions? Done. > >+ /* This is only used to call demote_(). */ > >+ nj::LIns *apply(nj::LIns *(*f)(nj::LirWriter *out, nj::LIns *ins), nj::LIns *ins) const { > >+ return f(lir, ins); > >+ } > > I'm probably missing a complication, but why can't Writer::demote() just call > demote_() directly? It can. I removed apply(), and renamed 'demote_' as 'Demote'. > >+ Writer(nj::Allocator *alloc_, nj::LirBuffer *lirbuf_) > >+ : alloc(alloc_), lirbuf(lirbuf_), lir(NULL), cse(NULL), logc(NULL) {} > > Usually, we don't decorate member variable names. Removed underscores. > Then, you can drop the somewhat unsightly: > > >+ nj::LIns *jfUnoptimizable(nj::LIns *cond) const { > >+ nj::LIns *jtUnoptimizable(nj::LIns *cond) const { > > since, with the declared MaybeJump type, there is local syntactic evidence that > the code is assuming the branch isn't being optimized away. I don't think that's going to work. I think you're assuming we can get away with a single version of jf(). I specifically want two different versions of jf() -- there are two different cases. > I think we could bring this comment into the type system a la: > > // outside Writer > struct AlreadyRootedOrNotGCThingPtr { void *p }; > struct NotGCThingPtr : AlreadyRootedOrNotGCThingPtr { /* ctor */ }; > struct AlreadyRootedGCThing : AlreadyRootedOrNotGCThingPtr { /* ctor */ }; I started doing this, then I realized that renaming immp() as immpNonGC() gives the same effect (the user has to type something indicating the lack of GC) without requiring extra types. So I did that.

(In reply to comment #9) > Done. I put it in the js::tl namespace, I hope this is ok. (It was unclear > to me why part of that file was in js::tl and part was only in js.) https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Namespaces 'tl' is for meta-functions (e.g. js::tl::StripConst<T>). Since this is a normal function, it should go in 'js'. > I don't think that's going to work. I think you're assuming we can get away > with a single version of jf(). I specifically want two different versions > of jf() -- there are two different cases. Fair enough. "Unoptimized" is a bit confusing though ("is this version slower than the normal jt?"). Perhaps "jtAlwaysDynamic" ? Your call. > I started doing this, then I realized that renaming immp() as immpNonGC() > gives the same effect (the user has to type something indicating the lack of > GC) without requiring extra types. So I did that. Ah, good point r+ Again, nice work.

> I was told to put "the Mozilla Corporation" here, For future ref, and we should really clean up existing variations, use Foundation not Corporation. The Foundation is the parent and sole stockholder. /be

Comment on attachment 487856 [details] [diff] [review] Merged to Sorry; bzexport put this in the wrong place.