Closed Bug 490296 Opened 13 years ago Closed 13 years ago

TraceMonkey: Tidy up of some ARM nanojit functions.

Categories

(Core :: JavaScript Engine, defect)

ARM
All
defect
Not set
minor

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.
Attached patch 1: Tidy ::asm_load64. (obsolete) — Splinter Review
Attachment #374746 - Flags: review?(vladimir)
Attachment #374746 - Attachment description: Tidy ::asm_load64. → 1: Tidy ::asm_load64.
Attached patch 2: Tidy ::asm_ld_imm. (obsolete) — Splinter Review
Attachment #374749 - Flags: review?(vladimir)
Attachment #374748 - Flags: review?(vladimir)
Attachment #374747 - Flags: review?(vladimir)
Attachment #374746 - Flags: review?(vladimir)
Attachment #374747 - Flags: review?(vladimir)
Attachment #374748 - Flags: review?(vladimir)
Attachment #374749 - Flags: review?(vladimir)
Sorry, I've broken something. I'll upload new patches shortly.
Attachment #374747 - Flags: review?(vladimir)
Attachment #374748 - Flags: review?(vladimir)
Attachment #374749 - Flags: review?(vladimir)
Attachment #374746 - Attachment is obsolete: true
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.
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)
Summary: TraceMonkey: Tidy-up of some ARM nanojit functions. → TraceMonkey: Tidy up of some ARM nanojit functions.
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.
Attachment #374774 - Flags: review?(vladimir)
Attachment #374776 - Flags: review?(vladimir)
Attached patch 7: Tidy ::asm_ldr_chk. (obsolete) — Splinter Review
Attachment #374885 - Flags: review?(vladimir)
Severity: normal → minor
Attached patch 8: Tidy ::asm_fop. (obsolete) — Splinter Review
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)
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.
Attachment #375010 - Flags: review?(vladimir)
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+
(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?
Attachment #374885 - Attachment is obsolete: true
How's this?
Attachment #374915 - Attachment is obsolete: true
Attachment #375156 - Flags: review?(vladimir)
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.
Attachment #375168 - Flags: review?(vladimir)
Cleaned up the code a bit as a mask isn't required when protected by isU8.
Attachment #374747 - Attachment is obsolete: true
Depends on: 487416
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+
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.
Keywords: checkin-needed
which of these patches need to be checked in ? :)
Comment on attachment 375156 [details] [diff] [review]
8: Tidy ::asm_fop (v2).

All of the r+'d ones :)
... 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.
This is why one patch per bug, or an mq stack of patches that you qfold before landing as one changeset, is best.

/be
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' :)
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..
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.
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
(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!
I merged all of these to trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.