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

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Ginn Chen]

This is a bug of the compiler.

If it is inside a namespace,
const int x = { 1 }; also fails.

Comment 1

[Ginn Chen]

Attachment: patch

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.

Comment 2

[Ginn Chen]

Attachment: patch v2

Hopefully it would be less messy and easier to maintain if I use a marco.
Also fix for SPARC.

Comment 3

[Edwin Smith]

Which backends actually have to compile with the Solaris Studio compiler?  I thought it would just be Sparc.

Comment 4

[Edwin Smith]

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.

Comment 5

[Rick Reitmaier]

(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 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Sparc-only patch (against TM 55344:db5b6e24f421)

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.

Comment 8

[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.

Comment 9

[Ginn Chen]

(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.

Comment 10

[Edwin Smith]

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.

Comment 11

[Ginn Chen]

Agree, which benchmark would you suggest?

Comment 12

[Nicholas Nethercote [inactive]]

Tracemonkey on Sunspider would be good, the compile-time is relatively high.

Comment 13

[Nicholas Nethercote [inactive]]

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().

Comment 14

[Ginn Chen]

Attachment: patch v3

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.

Comment 15

[Edwin Smith]

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.

Comment 16

[Ginn Chen]

(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 17

[Nicholas Nethercote [inactive]]

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 :)

Comment 18

[Ginn Chen]

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.

Comment 19

[Ginn Chen]

http://hg.mozilla.org/projects/nanojit-central/rev/c4f9bb1b1fd4

Comment 20

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/dd3ec76eacea

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/dd3ec76eacea

Comment 22

[Rick Reitmaier]

http://hg.mozilla.org/tamarin-redux/rev/699c70aacc79