Closed
Bug 490296
Opened 15 years ago
Closed 15 years ago
TraceMonkey: Tidy up of some ARM nanojit functions.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jbramley, Assigned: jbramley)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(10 files, 4 obsolete files)
735 bytes,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
1.57 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
917 bytes,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
1.73 KB,
patch
|
Details | Diff | Splinter Review | |
985 bytes,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
18.07 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
1023 bytes,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
These changes all came from a Tamarin patch which is described in bug 488826. The patch added VFP support to Tamarin, but also cleaned things up a bit, so I'm porting these to Trace Monkey. The attached patches are all small and don't really justify a bug to themselves, so I've grouped them here. Related bugs, which also came out of the Tamarin VFP patch: bug 489583, bug 488845.
Assignee | ||
Comment 1•15 years ago
|
||
Attachment #374746 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374746 -
Attachment description: Tidy ::asm_load64. → 1: Tidy ::asm_load64.
Assignee | ||
Comment 2•15 years ago
|
||
Assignee | ||
Comment 3•15 years ago
|
||
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #374749 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374748 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374747 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374746 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374747 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374748 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374749 -
Flags: review?(vladimir)
Assignee | ||
Comment 5•15 years ago
|
||
Sorry, I've broken something. I'll upload new patches shortly.
Assignee | ||
Updated•15 years ago
|
Attachment #374747 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374748 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374749 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374746 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
Comment on attachment 374746 [details] [diff] [review] 1: Tidy ::asm_load64. The ::asm_load64 function is either used differently in TM than in Tamarin, or the utility functions it uses behave differently. My ::asm_load64 patch is therefore invalid for TM, though the other three are Ok.
Assignee | ||
Comment 7•15 years ago
|
||
Fixed patch. This tidies asm_load64 a bit, but doesn't bring it much closer to Tamarin because doing so breaks things in TM.
Attachment #374756 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Summary: TraceMonkey: Tidy-up of some ARM nanojit functions. → TraceMonkey: Tidy up of some ARM nanojit functions.
Assignee | ||
Comment 8•15 years ago
|
||
Assignee | ||
Comment 9•15 years ago
|
||
This patch also adds proper support for loading small immediate values (rather than the x86-style XOR statement for the value 0). In addition, I've added some code to ensure that we don't generate loads that are out of range. This almost certainly never happened, but the code doesn't hurt and it's safer this way.
Assignee | ||
Updated•15 years ago
|
Attachment #374774 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Attachment #374776 -
Flags: review?(vladimir)
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #374885 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Severity: normal → minor
Assignee | ||
Comment 11•15 years ago
|
||
This one also appears to fix a bug, though I don't know if that bug was ever triggered. It's possible that TM's findRegFor is clever enough not to hit the bug anyway. (I've seen a few differences between the utility functions in TM and Tamarin.)
Attachment #374915 -
Flags: review?(vladimir)
Assignee | ||
Comment 12•15 years ago
|
||
This one just adds a comment, but the comment explains why the function is not used so it is valuable. The comment is taken directly from Tamarin but the references it makes still look valid for TM.
Assignee | ||
Updated•15 years ago
|
Attachment #375010 -
Flags: review?(vladimir)
Attachment #374747 -
Flags: review?(vladimir) → review+
Attachment #374748 -
Flags: review?(vladimir) → review+
Attachment #374749 -
Flags: review?(vladimir) → review+
Attachment #374756 -
Flags: review?(vladimir) → review+
Attachment #374774 -
Flags: review?(vladimir) → review+
Attachment #374776 -
Flags: review?(vladimir) → review+
Comment on attachment 374885 [details] [diff] [review] 7: Tidy ::asm_ldr_chk. >+#define isU12(offs) ((offs & 0xfff) == offs) Add some extra parens around both offs in the macro here... it might even be better to turn both of these into inline functions.
Attachment #374885 -
Flags: review?(vladimir) → review+
Comment on attachment 374915 [details] [diff] [review] 8: Tidy ::asm_fop. I'm not convinced that a switch() is tidier, but ok :)
Attachment #374915 -
Flags: review?(vladimir) → review+
Attachment #375010 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 15•15 years ago
|
||
(In reply to comment #13) > Add some extra parens around both offs in the macro here... it might even be > better to turn both of these into inline functions. Oops, missed those. Inline functions are usually preferable to macros, but in this case I wanted to remain consistent with the existing code. Converting macros to inline functions could be a good tidy-up patch in itself, but I'd rather do that as part of a separate bug. (In reply to comment #14) > (From update of attachment 374915 [details] [diff] [review]) > I'm not convinced that a switch() is tidier, but ok :) Agreed, though a switch is semantically cleaner. I could put each case on a single line if you like, or I could simply leave it as it is (but with the ~rmask(ra) addition). Which would you prefer?
Assignee | ||
Comment 16•15 years ago
|
||
Attachment #374885 -
Attachment is obsolete: true
Assignee | ||
Comment 17•15 years ago
|
||
How's this?
Attachment #374915 -
Attachment is obsolete: true
Attachment #375156 -
Flags: review?(vladimir)
Assignee | ||
Comment 18•15 years ago
|
||
This is a big one. Many of the changes are trivial and many are inter-dependent, so it's easier to just post one patch for this stuff.
Assignee | ||
Updated•15 years ago
|
Attachment #375168 -
Flags: review?(vladimir)
Assignee | ||
Comment 19•15 years ago
|
||
Cleaned up the code a bit as a mask isn't required when protected by isU8.
Attachment #374747 -
Attachment is obsolete: true
Comment 20•15 years ago
|
||
We used macros for a long time because C didn't have inline functions. In my opinion new code should use inline functions in preference to macros when you're not doing something macros can't do, like token-pasting; you won't see a patch (of mine, at least, I'm still trying to drag some of us kicking and screaming into 1999 :-) ) that adds the latter if the former is sufficient. If we weren't semi-locked-down I'd have started converting macros to inlines much more aggressively, but for now I generally only convert them as I make changes related to them.
Comment on attachment 375156 [details] [diff] [review] 8: Tidy ::asm_fop (v2). The switch is fine as it was (or this version, doesn't matter.. the other one's easier to edit if it's needed)
Comment on attachment 375168 [details] [diff] [review] 10: Tidy miscellaneous instruction generation macros. Looks fine -- let's get these checked in and do future cleanup stuff in a new bug, to avoid ballooning this bug with more patches :)
Attachment #375168 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 23•15 years ago
|
||
Yep; number 10 was the last of them. This is the last of the changes from the VFP patch for Tamarin. There's not actually any VFP improvement from these as we already have the VFP stuff.
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 24•15 years ago
|
||
which of these patches need to be checked in ? :)
Attachment #375156 -
Flags: review?(vladimir) → review+
Comment on attachment 375156 [details] [diff] [review] 8: Tidy ::asm_fop (v2). All of the r+'d ones :)
Assignee | ||
Comment 26•15 years ago
|
||
... plus those that replace r+ patches with minor changes: #2 and #7. They are numbered 1-10. It's probably best to commit them in that order because that's how I developed and tested them, though in practice they should apply anyway as they have mostly orthogonal changes. All patches that shouldn't be committed are marked as obsolete.
Comment 27•15 years ago
|
||
This is why one patch per bug, or an mq stack of patches that you qfold before landing as one changeset, is best. /be
Assignee | ||
Comment 28•15 years ago
|
||
Agreed. It seemed like a good idea at the time :-)
Not that we should be discussing it here, but I think the bunch-of-small-patches approach works here better than either of those, given that we have the obsolete flag, and qfold would lose the discrete steps involved here.. but for landing, attaching a hg bundle makes the landing job much easier. 'hg bundle mypatches.bundle http://hg.mozilla.org/tracemonkey' creates mypatches.bundle with all changesets not in the given remote; then the person applying can use 'hg unbundle mypatches.bundle' :)
Comment 30•15 years ago
|
||
NanoAssert fails on http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#1038 In LD32_nochk, LDR_nochk is called with PC. http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#1023
asm_ldr_chk already has an assert that r != IP in the case where it would break -- in the U12 case (which will be the case when loading from our const pool), we can do a PC-relative load just fine. I think we need to just get rid of that assert..
Comment 32•15 years ago
|
||
Ok, I got all the way up to the last two patches. #10 only has a few conflicts, but I don't understand them. The final #2 patch isn't reviewed. Should it be?
Comment on attachment 375172 [details] [diff] [review] 2: Tidy ::asm_ld_imm (v2). This is fine
Attachment #375172 -
Flags: review+
I just pushed #2 and #10, as well as the asm_ldr_chk fix for PC-relative loads.
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 35•15 years ago
|
||
(In reply to comment #30) > NanoAssert fails on > http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#1038 > > In LD32_nochk, LDR_nochk is called with PC. > http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#1023 Interesting; that didn't happen when I tested it. In any case, Vlad is right, and the existing assertion in the U12 case is sufficient. Thanks!
Comment 36•15 years ago
|
||
I merged all of these to trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•