Closed Bug 529430 Opened 10 years ago Closed 8 years ago

nanojit: expand PEDANTIC mode, or add STRESS mode


(Core Graveyard :: Nanojit, defect)

Not set


(Not tracked)



(Reporter: njn, Unassigned)




(1 file, 1 obsolete file)

Nanojit has a PEDANTIC compile-time mode defined in Native.h:

// define PEDANTIC=1 to ignore specialized forms, force general forms
// for everything, far branches, extra page-linking, etc.  This will
// flush out many corner cases.

There are two cases I'd like to either add to PEDANTIC, or have a similar mode for:

1. Make LIR code chunks as small as possible (LirBuffer::CHUNK_SZB).  I think the minimum possible will be 6 words, which is enough space for the biggest LIns (LInsSti) plus an LInsSk to link to the next chunk.

2. Make native code chunks as small as possible (pagesPerAlloc and bytesPerPage).  On X64 128 bytes works but 64 bytes causes crashes.

In both cases I'd like to work out the theoretical minimum and make sure it actually works in practice.  (Actually, for (2) I'd ideally like someone else to work out the theoretical minimum because I don't know CodeAlloc at all :)

Both of these make compilation much slower, but they will really flush out corner cases involving code chunking.  It's more of a STRESS test than a PEDANTIC test which is why I thought maybe a different mode would be useful, but fewer modes would be better so perhaps it should be folded into PEDANTIC.
Also, from bug 518928 (which I will close as a dup of this one):

"A useful way to stress the register allocator is to reduce the number of
registers available to the minimum.  Eg. on x86 I think this is eax/edx/ecx. 
But it requires modifying a couple of places.  So it would be good to have a
#define that can be set to turn this stress mode on, and define it for each

As part of this, we could have a static assert that ensures that the minimal
set of registers is available."
Duplicate of this bug: 518928
a few more categories: 

Worst case offsets for various cases of relative displacements:

  * load/store displacements
  * direct calls
  * native code page breaks (*)
  * branches to known targets
  * patching branches to unknown targets

(*) this is particularly important on PPC when the page-break-jump can clobber the same scratch register used by other instruction sequences that also count on being able to use the same scratch register.  Those sequences need a large enough underrunProtect to ensure no page break.

Worst case constant values (too big for immediate forms)

Regarding (2) above -- in the ppc and x64 backends, small code chunks are simulated in some #ifdef PEDANTIC mode in underrunProtect(), however we can make this much cleaner by passing the underrunProtect argument to codeAlloc and then let CodeAlloc dole out tiny chunks.  this not only makes lots of page breaks happen, but also inserts CodeAlloc's block headers inbetween tiny code chunks, making them subject to codeAlloc's sanity checks.

we could have STRESS_CODEALLOC, STRESS_LIRBUF, STRESS_REGALLOC, STRESS_DISPL, etc, so we can test each one separately during debugging.  typically we'd turn them all on, but if we find a bug it'd be nice to disable them one by one... staring at the output of STRESSED code and untangling what's going on will be really painful.
Attached patch STRESS_REGALLOC for i386 only (obsolete) — Splinter Review
It creates a zero-length array which I believe isn't standard C++ but is accepted by GCC at least.
Nope, 8.5.1 paragraph 4, "An array of unknown size initialized with a brace-enclosed initializer-list containing n initializers, where n shall be greater than zero".

I can't think of a reason why you couldn't always end the array with SENTINEL, keeping NumSavedRegs at its current value, at least not judging by an MXR case-insensitive search for "savedRegs".  Why isn't that possible?
It's possible, I didn't bother because this is code that will only be switched on occasionally for debugging purposes.
Depends on: 617244
A very rough patch, adding support for toggling stress mode at runtime.  Not ready for review yet.
Attachment #418942 - Attachment is obsolete: true
Notes to myself about the rough patch:

  - does Tamarin need any changes?
  - enable it from [after SSE/SSE2 clean-ups occur]
  - run on the Adobe try server
Assignee: nnethercote → general
Assignee: general → nobody
Component: JavaScript Engine → Nanojit
QA Contact: general → nanojit
Closed: 8 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.