491678 - TraceMonkey: Improve exit code efficiency on ARM.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jacob Bramley]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4
Build Identifier: 

This is a port of a Tamarin patch described by bug 445312. A detailed description of the change can be found there.

Note that I could not get all of the optimizations into TM. In particular, it should be possible to write the guard record pointer directly into R0, rather than putting it into R2 and then moving it in the epilogue. Indeed, it may be possible to do away with the epilogue altogether. However, the regular expression compiler in TM does something weird and for some reason it kicks up a fuss if I write to R0 in nFragExit. Tamarin didn't have a regular expression compiler (that I'm aware of) so it wasn't an issue.

The comments in my code should explain this.

I will attach my patch to this bug shortly.

Reproducible: Always

Comment 1

[Jacob Bramley [:jbramley]]

Sorry, I filed the bug under an old account which I'd forgotten about when I started using Bugzilla recently. It shouldn't matter, but that explains the different e-mail address.

Comment 2

[Jacob Bramley [:jbramley]]

Attachment: Improve exit code on ARM.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 375999 [details] [diff] [review]
Improve exit code on ARM.


Looks fine, though:

>+        } else if ((at[0] && 0xff000000) == (NIns)( COND_AL | (0xA<<24))) {
>+            // The existing branch looks like this:
>+            //  at[0]           B target
>+            //  at[1]           BKPT (dummy instruction).
>+            was = (NIns*) (((intptr_t)at + 8) + (intptr_t)((at[0] & 0xffffff) << 2));
>+        } else {
>+            // The existing code is not a branch. This can occur, for example,
>+            // when exiting from a fragment. Refer to nFragExit for details.
>+            was = 0;
>+        }

I'm not sure I understand this part; nFragExit always does a JMP_far, except for the LDi that it does -- is that what we'll get to here?  Is it really ok to just blow away whatever two instructions were here?

If this is the LDi and it's ok to blow it away, I'd much rather check for the LD instruction here... otherwise if we're given a bogus address to this function, we'll happily overwrite whatever's there.

Comment 4

[Jacob Bramley [:jbramley]]

Yep, it will be the LDi that I added to nFragExit earlier in the same patch. The new nFragExit sets gr->jmp to point to the LDi rather than the branch because the LDi isn't required if the branch is patched. It's safe to blow away the LDi because exits never appear to be unpatched.

I would also prefer to have an assertion in there. Indeed, I had to remove one which checked for a branch instruction. The problem is that LDi can emit several different instruction sequences and it is hard to write a reliable assertion. We can write one that works, but developers wouldn't expect to have to update nPatchBranch if they optimize something in LDi/asm_ld_imm, and because the output of LDi is platform- and context-dependent there's a good chance that errors in the assertion would be missed.

A nice workaround would be to insert a NOP to be overwritten, but there is an associated cost there. NOPs consume no cycles on newer cores but they still take a cache slot and use a word of memory.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, sounds fine then -- can you insert a comment to that effect?  Referring to nFragExit is fine, but nothing there explicitly answers the question of "what code am I looking at" in nPatchBranch :)

Comment 6

[Jacob Bramley [:jbramley]]

Attachment: Improve exit code on ARM (v2).

Good point! The attached patch has a more descriptive comment.

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a36145aabfa9