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)

defect
Not set
normal

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)

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.
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)
Target Milestone: --- → Future
Attached patch NJ patch (obsolete) — Splinter Review
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)
Attached patch TM patch (obsolete) — Splinter Review
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)
Attached patch TR patch (obsolete) — Splinter Review
Attachment #426110 - Flags: review?(edwsmith)
Attachment #426108 - Flags: review?(edwsmith)
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.
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.
(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 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+
Attachment #426109 - Flags: review?(nnethercote) → review+
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.
(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 :-)
(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?
(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.
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?
It still works in TR and it's still documented as working.  testing with the flag set to false is worryingly light, however.
Attached patch NJ v2Splinter Review
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)
Attached patch TR v2Splinter Review
Attachment #426110 - Attachment is obsolete: true
Attachment #426320 - Flags: review?(edwsmith)
Attachment #426110 - Flags: review?(edwsmith)
Attached patch TM v2Splinter Review
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)
Attachment #426319 - Flags: review?(edwsmith)
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+
Attachment #426323 - Flags: review?(nnethercote) → review+
(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.
(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.
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)
Attachment #426356 - Attachment is patch: true
Attachment #426356 - Attachment mime type: application/octet-stream → text/plain
Attachment #426356 - Flags: review?(edwsmith)
(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 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+
Blocks: 527258
(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.
Flags: flashplayer-qrb+
Attachment #426356 - Flags: review?(edwsmith) → review+
Attachment #426319 - Flags: review?(edwsmith) → review+
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+
(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.
http://hg.mozilla.org/tamarin-redux/rev/8ee29cea9d4a
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
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.

Attachment

General

Creator:
Created:
Updated:
Size: