521691 - NJ merge: tweaks found via lirasm --random

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

In order to land lirasm --random, I had to fix four assertion failures that it turned up.  This patch ports those fixes to TR.

Details, from bug 519873:

- Adds punctuation to clarify a comment in NativeX64.cpp.

- Fixes two assertions in NativeX64.cpp to allow obscure legitimate cases
  that --random threw up.

- Allows more than 5-bit offsets to LD8Z in Nativei386.h;  the comment
  talked about Thumb code and so presumably was accidentally copied from the
  ARM backend or something like that.

- Allows FP as the 2nd arg in SSE_MOVDm in Nativei386.h.  This was
  necessary for qlo to work (so presumably qlo isn't tested on i386).

Compiles cleanly, but I haven't tested it.

Comment 2

[Rick Reitmaier]

Comment on attachment 405807 [details] [diff] [review]
patch

+ing, but curious if we can add the assert back to LD8Z() .

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> +ing, but curious if we can add the assert back to LD8Z() .

There's no reason an LD8Z's offset has to be restricted to the range 0..31.  I hit numbers outside that range very easily and legitimately with lirasm --random, particularly negative offsets.  Also, the assertion isn't present in LD16Z.  I suspect the assertion hasn't been triggered until now because LD8Z is only used with LIR_ldcb, and LIR_ldcb is very rare.

(Actually, I see now that LD8Zdm and LD8Zsib should have the 0..31 assertion removed as well.)

This is one of the good things about lirasm --random, rare opcodes like LIR_ldcb get more of a workout.  Bug 520714 is another example, showing a problem with LIR_ldqc, another rare opcode.

Comment 4

[Steven Johnson]

(In reply to comment #3)
> (In reply to comment #2)
> > +ing, but curious if we can add the assert back to LD8Z() .
> 
> There's no reason an LD8Z's offset has to be restricted to the range 0..31.  

That is an historic artifact; LD8Z was originally added to nanojit to access some 8-bit fields in Tamarin that were declared in C++ structs. On ARM it's smaller code if the offset is 0..31 so the assert was added to ensure someone didn't move the field around in the struct. (Yeah, kind of a lame way to handle this.)

Comment 5

[Rick Reitmaier]

pushed a slightly modified version of this patch labeled under bug 519873

http://hg.mozilla.org/tamarin-redux/rev/0992abc3074f

Comment 6

[Chris Peyer]

Engineering work item.  Marking as verified.