Closed Bug 602388 Opened 9 years ago Closed 9 years ago

nanojit: make Register a non-numeric type on Sparc

Categories

(Core Graveyard :: Nanojit, defect)

Sun
OpenSolaris
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: njn, Assigned: ginnchen+exoracle)

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.
We need to fix the compile issue of Solaris Studio first.
See Bug 599251 comment #24

What's the best way to fix that?
Use same definition of #elif defined(DEBUG) for Solaris Studio?
i.e.
Struct Register {
  uint32_t n;
}
I think thats the most practical workaround.  If the compiler can successfully pass Register struct values as register parameters, and manipulate them in registers, then it's not only a workaround, but a good fix in general.  The reason for defining Register both ways is to side-step optimizers that fail to put word-sized struct values in registers.
I did some tests, I found Solaris Studio is not using register for Register struct. :(
So we will miss some optimizations.

An ugly workaround would be remove "const" for rEAX, rECX, etc. //sigh
(In reply to comment #3)
> I did some tests, I found Solaris Studio is not using register for Register
> struct. :(
> So we will miss some optimizations.

Man, what a great compiler :P

Fortunately Ed (or was it Rick?) anticipated this, and if you look at RegisterCommon.h you'll see that (once NJ_USE_STRUCT_REGISTER is defined) that struct Register is only used in debug builds.  It's still an integer in opt builds.
The discussion of compiler bug moved to Bug 603560.


I'm working on this bug, it is not as easy as I thought.
For SPARC, marcos are using.
This is a mess, because sometimes "rd" is a register, sometimes it is a number.
We might want to change these things to inline functions like x86.
OS: Mac OS X → Solaris
Hardware: x86 → Sun
(In reply to comment #5)
> 
> I'm working on this bug, it is not as easy as I thought.
> For SPARC, marcos are using.
> This is a mess, because sometimes "rd" is a register, sometimes it is a number.
> We might want to change these things to inline functions like x86.

Yes, changing from macros to inline functions will make things a lot easier.  
For the ARM backend you can see that bug 602386 depends on bug 592223, you might want to file a similar bug.  The conversion from macros to functions is tedious but not difficult, and makes things much more type-safe and readable.

The i386 backend also has cases where a value is sometimes a register, sometimes a number... the basic approach is this: for a value which can be either, make it an int32_t.
Depends on: 603560
Attached patch patch (obsolete) — Splinter Review
I changed RegisterMask to uint64_t for SPARC, thus we can use 32-63 for F0-F32.
Therefore I need to slightly change Assembler.cpp, Nativei386.h.
Attachment #484258 - Flags: review?(nnethercote)
Comment on attachment 484258 [details] [diff] [review]
patch

>diff --git a/nanojit/Assembler.cpp b/nanojit/Assembler.cpp

>     // Scan table for instruction with the lowest priority, meaning it is used
>     // furthest in the future.
>     LIns* Assembler::findVictim(RegisterMask allow)
>     {
>         NanoAssert(allow);
>         LIns *ins, *vic = 0;
>-        int allow_pri = 0x7fffffff;
>+        RegisterMask allow_pri = ~RegisterMask(0);
>         RegisterMask vic_set = allow & _allocator.activeMask();
>         for (Register r = lsReg(vic_set); vic_set; r = nextLsReg(vic_set, r))

In this code, allow_pri is not a mask, it is a "priority" value, which is just an incremented integer.  The priority and usepri[] fields in RegAlloc are intentionally plain integers.  So I don't think you should need to change it.
Attached patch patchSplinter Review
You're right, I only need to change NativeSparc.*
It must be a mistake in debugging.
Attachment #484258 - Attachment is obsolete: true
Attachment #484603 - Flags: review?(nnethercote)
Attachment #484258 - Flags: review?(nnethercote)
Comment on attachment 484603 [details] [diff] [review]
patch

Looks fine.  I'd put a blank line between each function though, except for those ones where the whole function is on a single line.
Attachment #484603 - Flags: review?(nnethercote) → review+
Committed with blank lines and a few small tweaks.

http://hg.mozilla.org/projects/nanojit-central/rev/8f528c838dae
OS: Solaris → OpenSolaris
Whiteboard: fixed-in-nanojit
remove inline keyword for JMP() to fix bustage of tracemonkey
Sorry I used wrong bug number in hg. :(

http://hg.mozilla.org/projects/nanojit-central/rev/7abdd4e5c5df
http://hg.mozilla.org/tracemonkey/rev/cc0c87e2dd28
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/cc0c87e2dd28
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/tamarin-redux/rev/db7a0c552b5d
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.