Closed Bug 603560 Opened 9 years ago Closed 9 years ago

Solaris Studio failed to compile static const int x = { 1 };

Categories

(Core Graveyard :: Nanojit, defect)

All
OpenSolaris
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

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

Attachments

(1 file, 3 obsolete files)

This is a bug of the compiler.

If it is inside a namespace,
const int x = { 1 }; also fails.
Attached patch patch (obsolete) — Splinter Review
Sorry for the mess.
Can you live with it?
 
Remove the workaround for Bug 544447. 
"=a"(r) doesn't work if r is a struct.
"=m"(r) would work.
Since I can no longer see the bug, we can just remove it.
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #482480 - Flags: review?(edwsmith)
Attached patch patch v2 (obsolete) — Splinter Review
Hopefully it would be less messy and easier to maintain if I use a marco.
Also fix for SPARC.
Attachment #482480 - Attachment is obsolete: true
Attachment #482550 - Flags: review?(edwsmith)
Attachment #482480 - Flags: review?(edwsmith)
Which backends actually have to compile with the Solaris Studio compiler?  I thought it would just be Sparc.
OS: Solaris → Android
Blocks: 602384
Comment on attachment 482550 [details] [diff] [review]
patch v2

I think its better if I delegate this review to Nick.

The idea of using a macro to initialize registers came up before, and got rejected before we knew about this bug in the Solaris Studio compiler.

Here is the previous thread.
https://bugzilla.mozilla.org/show_bug.cgi?id=599251#c12

Could we work around the bug by using a macro for the "static const" part?

#ifdef broken_compiler
# define REGISTER_CONST static
#else
# define REGISTER_CONST static const
#endif

Then, we'd only have to use REGISTER_CONST.  Just an idea, maybe no better since that macro would show up everywhere the REGISTER_INIT macro would have to show up.
Attachment #482550 - Flags: review?(edwsmith) → review?(nnethercote)
(In reply to comment #4)
> Comment on attachment 482550 [details] [diff] [review]
> patch v2
> 

This probably came up already but what about using a ctor in the struct, like so:

    struct Register {
        uint32_t n;     // the register number
        inline Register(uint32_t i) { n = i; }
    };

Or do we have some compilers that aren't swift enough to realize that this can be optimized to a C-style struct.
Comment on attachment 482550 [details] [diff] [review]
patch v2

Non-Sparc backends shouldn't have to see this workaround.  I'll attach a
patch that only changes the Sparc backend.
Attachment #482550 - Flags: review?(nnethercote) → review-
I haven't tested this as I don't have access to a Sparc box, but I think it should work with at most minor corrections.  Ginn, can you test it?  Thanks.
Attachment #482691 - Flags: review?(ginn.chen)
(In reply to comment #3)
> Which backends actually have to compile with the Solaris Studio compiler?  I
> thought it would just be Sparc.

No, it's not just SPARC.
We use Solaris Studio to compile Firefox and Flashplayer on x86, x64 and SPARC.
gcc is poorly supported on Solaris platform.

(In reply to comment #4)
> Could we work around the bug by using a macro for the "static const" part?
> 
> #ifdef broken_compiler
> # define REGISTER_CONST static
> #else
> # define REGISTER_CONST static const
> #endif
> 

Yes, I thought about it.
But it's less optimized, too.
I don't have the performance comparison number because I don't know how to do it with nanojit-central.
From the code size, it is larger.
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 482550 [details] [diff] [review] [details]
> > patch v2
> > 
> 
> This probably came up already but what about using a ctor in the struct, like
> so:
> 
>     struct Register {
>         uint32_t n;     // the register number
>         inline Register(uint32_t i) { n = i; }
>     };
> 
> Or do we have some compilers that aren't swift enough to realize that this can
> be optimized to a C-style struct.

The problem is we'll lose half of the benefits of Bug 599251.
What if, on Solaris Studio compiler (x86-32, x86-64, Sparc), you run some benchmarks with Register defined as a struct (as proposed in comment #1 on bug 602388).  In theory, the JIT code backend will be slower due to worse register allocation, but we don't know by how much, and (optimistically) it may not be a huge impact.

This would allow all the backends to avoid macros, and once the SS compiler is fixed, the header can be changed to use uint32_t in optimized builds.  In other words, the price of this SS bug is paid by SS builds of the JIT being a bit slower, without impacting the source code for everyone else.
Agree, which benchmark would you suggest?
Tracemonkey on Sunspider would be good, the compile-time is relatively high.
Maybe it's just easier to avoid the "x = { 1 }" syntax altogether.  Like Ginn's v2 patch, but doing it everywhere and avoiding SUNPRO #ifdefs.  And maybe something shorter than REGVALUE(), eg. MKREG().
Attached patch patch v3Splinter Review
Use the struct declaration as a workaround, the performance difference is not noticeable.

BTW: the "static uint32_t" workaround will cost about 2% slower for sunspider with tracejit.

Note: this patch will not fix build issue of SPARC platform until Bug 602388 is fixed.
Attachment #482550 - Attachment is obsolete: true
Attachment #482691 - Attachment is obsolete: true
Attachment #483118 - Flags: review?(edwsmith)
Attachment #482691 - Flags: review?(ginn.chen)
Blocks: 602388
No longer blocks: 602384
Blocks: 602384
Comment on attachment 483118 [details] [diff] [review]
patch v3

(In reply to comment #14)
> Created attachment 483118 [details] [diff] [review]
> patch v3
> 
> Use the struct declaration as a workaround, the performance difference is not
> noticeable.

Great!  This patch looks quite simple.

> BTW: the "static uint32_t" workaround will cost about 2% slower for sunspider
> with tracejit.

Good to know.  But it sounds like we no longer need that workaround.  Do I understand correctly?

I don't see anything wrong with this patch, but since I delegated to Nick before, it seems proper to ask for feedback on this too.  If this doesn't hurt performance and it doesn't require a macro, I like it.
Attachment #483118 - Flags: review?(edwsmith)
Attachment #483118 - Flags: review+
Attachment #483118 - Flags: feedback?(nnethercote)
(In reply to comment #15)
> > BTW: the "static uint32_t" workaround will cost about 2% slower for sunspider
> > with tracejit.
> 
> Good to know.  But it sounds like we no longer need that workaround.  Do I
> understand correctly?


Right.
Comment on attachment 483118 [details] [diff] [review]
patch v3

>+#elif defined(DEBUG) || defined(__SUNPRO_CC)
>+    // Always use struct declaration for 'Register' with
>+    // Solaris Studio C++ compiler, because it has a bug:
>+    // Scaler type can not be initialized by '{1}'.
>+    // See bug 603560.

It's "scalar", not "scaler".


>-    #elif defined __SUNPRO_CC
>-        // Workaround for Sun Studio bug on handler embeded asm code.
>-        // See bug 544447 for detail.
>-        // https://bugzilla.mozilla.org/show_bug.cgi?id=544447
>-         asm(
>-             "bsf    %1, %%edi\n\t"
>-             "btr    %%edi, (%2)\n\t"
>-             "movl   %%edi, %0\n\t"
>-             : "=a"(r) : "d"(set), "c"(&regs.free) : "%edi", "memory" );

This seems unrelated, but I'm always happy to remove SUNPRO-specific code :)
Attachment #483118 - Flags: feedback?(nnethercote) → feedback+
It is related, because you cannot use "=a"(r) when r is a struct.
I'm removing it since I cannot recreate the bug now.
OS: Android → OpenSolaris
http://hg.mozilla.org/tracemonkey/rev/dd3ec76eacea
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/dd3ec76eacea
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/tamarin-redux/rev/699c70aacc79
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.