Closed
Bug 542133
Opened 14 years ago
Closed 14 years ago
Add a real NJConfig struct to nanojit
Categories
(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)
Tamarin Graveyard
Baseline JIT (CodegenLIR)
Tracking
(Not tracked)
RESOLVED
FIXED
Future
People
(Reporter: stejohns, Unassigned)
References
Details
(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)
Attachments
(4 files, 3 obsolete files)
50.41 KB,
patch
|
n.nethercote
:
review+
edwsmith
:
review+
|
Details | Diff | Splinter Review |
17.74 KB,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
6.27 KB,
patch
|
n.nethercote
:
review+
edwsmith
:
review+
|
Details | Diff | Splinter Review |
Currently you set the runtime options for nanojit by passing a type of "AvmCore::Config", but no such uniform type exists in nanojit; instead, there are two completely unrelated structs in TM and TR that are coordinated to provide structs that happen to have fields with similar enough types to match. This is nasty; we should make an explicit struct in (say) nanojit.h and require TR and TM to fill it in explicitly.
Reporter | ||
Comment 1•14 years ago
|
||
While we're at it, perhaps we should move the various detection bits (sse2, ARM arch version, ARM vfp-or-not) into nanojit itself rather than replicating that code into TR and TM... then this could be used to fill in useful defaults for NJConfig. (Embedder could still override these, of course)
Reporter | ||
Comment 2•14 years ago
|
||
First cut at this: moves the existing NJ-specific flags into a new "Config" struct (in njconfig.h), and adds a ctor for that struct that attempts to fill in safe values based on compiler flags. (A useful subsequent patch might be to move the runtime sniffing code from TraceMonkey into nanojit.)
Attachment #426108 -
Flags: review?(nnethercote)
Reporter | ||
Comment 3•14 years ago
|
||
TraceMonkey patch. This could probably be modified further in the name of cleanliness (eg, there's no need for the bogus "avmplus::AvmCore::" namespacing anymore) but I opted to make the patch bare-minimum for now to get feedback on the general approach.
Attachment #426109 -
Flags: review?(nnethercote)
Reporter | ||
Comment 4•14 years ago
|
||
Attachment #426110 -
Flags: review?(edwsmith)
Reporter | ||
Updated•14 years ago
|
Attachment #426108 -
Flags: review?(edwsmith)
Reporter | ||
Comment 5•14 years ago
|
||
Note 1: There were various config flags that appeared to be completely unused (eg "quiet_opt"), so they were eliminated. If they are used somewhere that isn't apparent from TM+TR, let me know. Note 2: There is a "cseopt" setting to enable CSE (default=true); I neglected to configure TM to actually use this flag, so that's a known defect in the TM patch.
Comment 6•14 years ago
|
||
Another approach would be to move config into the NativeXXX.cpp/h files? Then we don't need to have a njconfig file with a smattering of ifdefs in it. Or we could use a pattern like DECLARE_PLATFORM_ASSEMBLER() in assembler. Where Assembler.h contains the 'base' structure and each back-end can add its own platform specific fields.
Reporter | ||
Comment 7•14 years ago
|
||
(In reply to comment #6) > Or we could use a pattern like DECLARE_PLATFORM_ASSEMBLER() in assembler. > Where Assembler.h contains the 'base' structure and each back-end can add its > own platform specific fields. Sure, that would work too. Given the current number of platform-specific configs (small) it may be premature optimization, though. I think TR's internal config struct used to be all ifdef'ed and we actually ripped 'em out in the name of readability.
Comment 8•14 years ago
|
||
Comment on attachment 426108 [details] [diff] [review] NJ patch >+#ifdef NANOJIT_IA32 >+ x86_sse2 = CheckForSSE2(); >+ x86_use_cmov = true; >+ x86_fixed_esp = false; >+#endif Can we use i386_xyz to match Nativei386.cpp, please? r=me with that fixed. (If you want to change NANOJIT_IA32 to NANOJIT_I386 that's fine by me too :) Nice patch.
Attachment #426108 -
Flags: review?(nnethercote) → review+
Updated•14 years ago
|
Attachment #426109 -
Flags: review?(nnethercote) → review+
Comment 9•14 years ago
|
||
Comment on attachment 426109 [details] [diff] [review] TM patch > Note 2: There is a "cseopt" setting to enable CSE > (default=true); I neglected to configure TM to actually use > this flag, so that's a known defect in the TM patch. Eh, that doesn't worry me much. TM always wants CSE on.
Reporter | ||
Comment 10•14 years ago
|
||
(In reply to comment #9) > Eh, that doesn't worry me much. TM always wants CSE on. TR does do -- it's mainly an internal debugging tool. (In reply to comment #8) > Can we use i386_xyz to match Nativei386.cpp, please? Sure. > (If you want to change NANOJIT_IA32 to NANOJIT_I386 that's fine by me too :) Scope creep :-)
Comment 11•14 years ago
|
||
(In reply to comment #10) > > Eh, that doesn't worry me much. TM always wants CSE on. > > TR does do -- it's mainly an internal debugging tool. Seems like a crappy one. You have to change a line and then recompile everything because a header has changed? Whereas if you just changed the relevant line in CodegenLIR.cpp you just have to recompile that file?
Reporter | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > Seems like a crappy one. You have to change a line and then recompile > everything because a header has changed? Whereas if you just changed the > relevant line in CodegenLIR.cpp you just have to recompile that file? No, we don't have to recompile anything -- we can change a config file or env var to clear the flag.
Reporter | ||
Comment 13•14 years ago
|
||
It does bring up the question of whether we really need this flag anymore -- I think it's a holdover from debugging early versions of MIR (!). Edwin?
Comment 14•14 years ago
|
||
It still works in TR and it's still documented as working. testing with the flag set to false is worryingly light, however.
Reporter | ||
Comment 15•14 years ago
|
||
Revised patch -- misc cleanup from running thru buildbots, renaming per Nick's request, and moved the arm_arch sniffing into njconfig.h, as TR is likely to want to use it in a different context and there's little point in having to replicate it.
Attachment #426108 -
Attachment is obsolete: true
Attachment #426319 -
Flags: review?(nnethercote)
Attachment #426108 -
Flags: review?(edwsmith)
Reporter | ||
Comment 16•14 years ago
|
||
Attachment #426110 -
Attachment is obsolete: true
Attachment #426320 -
Flags: review?(edwsmith)
Attachment #426110 -
Flags: review?(edwsmith)
Reporter | ||
Comment 17•14 years ago
|
||
Note that this adds an "if cseopt" for CseFilter, for completeness. Feel free to strip it before landing if you don't want it possible to disable it.
Attachment #426109 -
Attachment is obsolete: true
Attachment #426323 -
Flags: review?(nnethercote)
Reporter | ||
Updated•14 years ago
|
Attachment #426319 -
Flags: review?(edwsmith)
Comment 18•14 years ago
|
||
Comment on attachment 426319 [details] [diff] [review] NJ v2 One question: as far as I can tell you've preserved the behaviour of tree_opt -- it's false for TM and TR, but true for lirasm. Is that right? Seems a bit strange -- why does lirasm need it?
Attachment #426319 -
Flags: review?(nnethercote) → review+
Updated•14 years ago
|
Attachment #426323 -
Flags: review?(nnethercote) → review+
Reporter | ||
Comment 19•14 years ago
|
||
(In reply to comment #18) > (From update of attachment 426319 [details] [diff] [review]) > One question: as far as I can tell you've preserved the behaviour of tree_opt > -- it's false for TM and TR, but true for lirasm. Is that right? Seems a bit > strange -- why does lirasm need it? Interesting, I must have missed that; I thought that TM set it to true as well. I actually don't really know why lirasm needs it.
Comment 20•14 years ago
|
||
(In reply to comment #19) > > > > One question: as far as I can tell you've preserved the behaviour of tree_opt > > -- it's false for TM and TR, but true for lirasm. Is that right? Seems a bit > > strange -- why does lirasm need it? > > Interesting, I must have missed that; I thought that TM set it to true as well. > I actually don't really know why lirasm needs it. Graydon, Jason: do you know what tree_opt does (something to do with fragment exits on i386 and MIPS, but I don't know any more than that) and why it's on for lirasm but off for TM and TR? Thanks.
Reporter | ||
Comment 21•14 years ago
|
||
Sorry for the patch churn... this is another patch (additive to the previous NJ patch) that splits the NJ_COMPILER_ARM_ARCH macro into its own file. I'd like to recycle that detection in TR, but I need it in a place that is before avmplus.h is included, so I need to move the logic into a naked file.
Attachment #426356 -
Flags: review?(nnethercote)
Reporter | ||
Updated•14 years ago
|
Attachment #426356 -
Attachment is patch: true
Attachment #426356 -
Attachment mime type: application/octet-stream → text/plain
Attachment #426356 -
Flags: review?(edwsmith)
Comment 22•14 years ago
|
||
(In reply to comment #20) > (In reply to comment #19) > Graydon, Jason: do you know what tree_opt does (something to do with fragment > exits on i386 and MIPS, but I don't know any more than that) and why it's on > for lirasm but off for TM and TR? Thanks. Has to do with compiling multiple fragments in one go. As far as I know it's dead code at this point, though it's been a while since I looked. Possibly Edwin can comment on its present or future viability?
Comment 23•14 years ago
|
||
Comment on attachment 426356 [details] [diff] [review] Additional NJ tweak FWIW, if I get an r+ on a patch and then I see a tiny extra change required I usually don't bother asking for another review.
Attachment #426356 -
Flags: review?(nnethercote) → review+
Comment 24•14 years ago
|
||
(In reply to comment #22) > (In reply to comment #20) > Has to do with compiling multiple fragments in one go. As far as I know it's > dead code at this point, though it's been a while since I looked. Possibly > Edwin can comment on its present or future viability? A very long time ago, that option in TT decided whether we would jit and patch one trace branch at a time, or recompile the whole tree of LIR each time a branch was added. The experiment failed -- recompile times went way up, performance did not, and that was the last I ever looked at the tree_opt option. I would imagine if TM wants to experiment with whole-tree recompilation, a lotta new code would need to be written and tweaked. I vote remove it unless it's somehow essential for lirasm.
Updated•14 years ago
|
Attachment #426356 -
Flags: review?(edwsmith) → review+
Updated•14 years ago
|
Attachment #426319 -
Flags: review?(edwsmith) → review+
Comment 25•14 years ago
|
||
Comment on attachment 426320 [details] [diff] [review] TR v2 nit: CodegenLIR.cpp:5527 there's a lot going in in this method, if you can find another place to put this sniff-test, super. maybe in Codemgr's constructor since it is infrequent.
Attachment #426320 -
Flags: review?(edwsmith) → review+
Reporter | ||
Comment 26•14 years ago
|
||
(In reply to comment #24) > I vote remove it unless it's somehow essential for lirasm. I will nuke it before pushing.(In reply to comment #25) > (From update of attachment 426320 [details] [diff] [review]) > nit: deal.
Reporter | ||
Comment 27•14 years ago
|
||
http://hg.mozilla.org/projects/nanojit-central/rev/353c06e32329
Whiteboard: fixed-in-nanojit
Comment 28•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/42c01cffd7bc http://hg.mozilla.org/tracemonkey/rev/7c63a6c5ca78
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Reporter | ||
Comment 29•14 years ago
|
||
http://hg.mozilla.org/tamarin-redux/rev/8ee29cea9d4a
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Comment 30•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/42c01cffd7bc
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•