TraceMonkey: Improve exit code efficiency on ARM.

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: jacob.bramley, Assigned: jbramley)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Assignee)

Updated

10 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

10 years ago
Assignee: general → Jacob.Bramley
(Assignee)

Comment 1

10 years ago
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.
(Assignee)

Comment 2

10 years ago
Created attachment 375999 [details] [diff] [review]
Improve exit code on ARM.
(Assignee)

Updated

10 years ago
Attachment #375999 - Flags: review?(vladimir)
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.
Attachment #375999 - Flags: review?(vladimir) → review+
(Assignee)

Comment 4

10 years ago
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.
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 :)
(Assignee)

Comment 6

10 years ago
Created attachment 376386 [details] [diff] [review]
Improve exit code on ARM (v2).

Good point! The attached patch has a more descriptive comment.
Attachment #375999 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey

Comment 7

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