If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add a real NJConfig struct to nanojit

RESOLVED FIXED in Future

Status

Tamarin
Baseline JIT (CodegenLIR)
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Steven Johnson, Unassigned)

Tracking

unspecified
Future
Bug Flags:
flashplayer-qrb +

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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

8 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)

Updated

8 years ago
Target Milestone: --- → Future
(Reporter)

Comment 2

8 years ago
Created attachment 426108 [details] [diff] [review]
NJ patch

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

8 years ago
Created attachment 426109 [details] [diff] [review]
TM patch

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

8 years ago
Created attachment 426110 [details] [diff] [review]
TR patch
Attachment #426110 - Flags: review?(edwsmith)
(Reporter)

Updated

8 years ago
Attachment #426108 - Flags: review?(edwsmith)
(Reporter)

Comment 5

8 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

8 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

8 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 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.
(Reporter)

Comment 10

8 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 :-)
(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

8 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

8 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

8 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

8 years ago
Created attachment 426319 [details] [diff] [review]
NJ v2

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

8 years ago
Created attachment 426320 [details] [diff] [review]
TR v2
Attachment #426110 - Attachment is obsolete: true
Attachment #426320 - Flags: review?(edwsmith)
Attachment #426110 - Flags: review?(edwsmith)
(Reporter)

Comment 17

8 years ago
Created attachment 426323 [details] [diff] [review]
TM v2

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

8 years ago
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+
(Reporter)

Comment 19

8 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.
(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

8 years ago
Created attachment 426356 [details] [diff] [review]
Additional NJ tweak

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

8 years ago
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+
(Reporter)

Updated

8 years ago
Blocks: 527258

Comment 24

8 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

8 years ago
Flags: flashplayer-qrb+

Updated

8 years ago
Attachment #426356 - Flags: review?(edwsmith) → review+

Updated

8 years ago
Attachment #426319 - Flags: review?(edwsmith) → review+

Comment 25

8 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

8 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

8 years ago
http://hg.mozilla.org/projects/nanojit-central/rev/353c06e32329
Whiteboard: fixed-in-nanojit
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

8 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

8 years ago
http://hg.mozilla.org/mozilla-central/rev/42c01cffd7bc
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.