Closed Bug 602387 Opened 9 years ago Closed 9 years ago

nanojit: make Register a non-numeric type on PPC

Categories

(Core Graveyard :: Nanojit, defect)

PowerPC
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: njn, Assigned: edwsmith)

References

Details

(Whiteboard: fixed-in-nanojit,fixed-in-tracemonkey,fixed-in-tamarin)

Attachments

(1 file, 1 obsolete file)

Bug 599251 introduced the non-numeric Register type.  This back-end doesn't use it yet.  It should.  See the i386/X64 back-ends for examples.
Hardware: x86 → PowerPC
Target Milestone: --- → Future
Depends on: 599251
Attached patch PPC fixes, plus operator<=() (obsolete) — Splinter Review
Nick, let me know if you have any concerns about operator<=(Register,Register), it simplified the logic for PPC asm_call().  I didn't add the other comparison operators because they would be dead code, but its a bit weird without them.
Attachment #483154 - Flags: review?(rreitmai)
Attachment #483154 - Flags: feedback?(nnethercote)
Comment on attachment 483154 [details] [diff] [review]
PPC fixes, plus operator<=()

>                 #ifdef NANOJIT_64BIT
>-                    r = Register(r + 1);
>+                    r = REGINC(r);
>                 #else
>-                    r = Register(r + 2); // skip 2 gpr's
>+                    Register rnext = { REGNUM(r) + 2 }; // skip 2 gpr's
>+                    r = rnext;
>                 #endif
>                     param_size += sizeof(double);
>                 } else {

Actually, can we get rid of REGINC and just use "{ REGNUM(r) + 1 }" here and in the X64 backend?  I know I introduced it, I think it's error prone because it's unclear if it updates it's argument or not.  (When I first converted the X64 back-end, I introduced a bug because I forgot the "r =" at the front.


>     static inline bool IsGpReg(Register r) {
>-        return r <= R31;
>+        return REGNUM(r) <= REGNUM(R31);
>     }
>     static inline bool IsFpReg(Register r) {
>-        return r >= F0;
>+        return REGNUM(r) >= REGNUM(F0);
>     }

Can't you use the Register <= here?

I'm fine with the overloaded <=, BTW, I still think of the Register type as an integer, just one that can't be mixed up with other integers.
Attachment #483154 - Flags: feedback?(nnethercote) → feedback+
> Actually, can we get rid of REGINC and just use "{ REGNUM(r) + 1 }" here and in
> the X64 backend?  I know I introduced it, I think it's error prone because it's
> unclear if it updates it's argument or not.  (When I first converted the X64
> back-end, I introduced a bug because I forgot the "r =" at the front.

I made the same mistake, too.  I'll kill REGINC and change the backends.

> I'm fine with the overloaded <=, BTW, I still think of the Register type as an
> integer, just one that can't be mixed up with other integers.

Willfix... that was leftover from before i added operator<=()
Comment on attachment 483154 [details] [diff] [review]
PPC fixes, plus operator<=()

If we ok with overloading then operator+ can replace REGINC.
Attachment #483154 - Flags: review?(rreitmai) → review+
At first I wasn't going to add operator+, but in looking at the examples where we have

   Register t = { r + c };
   /* use t */

I decided to bite the bullet and add operator+.  Also, I found a couple more places where other relational operators were helpful.

This patch is right at the edge of "scope creep", but I think it improves the code more than it hurts.  It removes REGINC, introduces the remaining relational operators, and operator+, and uses them in NativeX64.cpp, NativeSparc.cpp (where REGINC was used several times), and NativePPC.cpp.
Attachment #483154 - Attachment is obsolete: true
Attachment #486976 - Flags: review?(nnethercote)
Attachment #486976 - Flags: feedback?(rreitmai)
Attachment #486976 - Flags: feedback?(ginn.chen)
Comment on attachment 486976 [details] [diff] [review]
PPC fixes, remove REGINC, add operator +, <=, >=, <, >

>diff -r f94567de6373 nanojit/NativeCommon.h
>--- a/nanojit/NativeCommon.h	Fri Oct 29 09:54:10 2010 -0400
>+++ b/nanojit/NativeCommon.h	Fri Oct 29 14:48:54 2010 -0400
>@@ -75,8 +75,10 @@
>         return r.n;
>     }
> 
>-    static inline Register REGINC(Register r) {
>-        r.n++;
>+    /** This is intended to support (r + const) expressions. */
>+    static inline Register operator+(Register r, int c)
>+    {
>+        r.n += c;

Nit: change /** to /*, please.

Actually, you could remove that whole comment, it's rather obvious :)
Attachment #486976 - Flags: review?(nnethercote) → review+
Attachment #486976 - Flags: feedback?(ginn.chen) → feedback+
Okay, I'll remove it.  I've started getting in the habit of using doc comments (for doxygen) but its out of place for just this one case.

Also, in case you were wondering, I did not add operator+= or ++, because I felt like we should "draw the line in the sand" somewhere.  If anyone feels strongly they should be there for completeness, speak up.

Operators that return Register are probably safe.  Operators that return int, given registers, would not be -- they amount to an implicit cast, which we are purposefully avoiding.
Comment on attachment 486976 [details] [diff] [review]
PPC fixes, remove REGINC, add operator +, <=, >=, <, >

F+ much cleaner code to these eyes.
Attachment #486976 - Flags: feedback?(rreitmai) → feedback+
NJ: http://hg.mozilla.org/projects/nanojit-central/rev/7bec0eb6482c
Assignee: edwsmith → nobody
Whiteboard: fixed-in-nanojit
http://hg.mozilla.org/tracemonkey/rev/322fe3e6482e
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
edwsmith http://hg.mozilla.org/tamarin-redux/rev/f0edd5fee07e
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit,fixed-in-tracemonkey,fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/322fe3e6482e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee: nobody → edwsmith
Target Milestone: Future → mozilla2.0b8
Version: unspecified → Trunk
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.