1339697 - Give Register a constexpr constructor

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[(Away)]

Attachment: Give Register a constexpr constructor

Visual Studio is producing a bazillion distinct copies of constants like WasmTableCallScratchReg, one for every translation unit containing that header, and a runtime (!) initializer function for each.

I followed the chain of assignments (WasmTableCallScratchReg <- ABINonArgReg0 <- rax) and I suspect that this happens at runtime because VS can't be sure that `rax` can be initalized at compile-time.

Giving `Register` a constexpr constructor works around the problem and removes 36k from xul.dll! Most of that comes from removing the init functions, but we also drop those values from .data and just incorporate the constants into the code.

There's a small snag in that the presence of a user-provided constructor precludes aggregate-initialization, so now code like:
        Register r = { Encoding(i) };
is considered an initializer-list rather than an aggregate-initialization, and that can't go through the explicit constructor. So I've had to patch up a few callers (either that or we mark the constructor MOZ_IMPLICIT, but I believe that's frowned upon).

The presence of a user-provided constructor also removes the default constructor, which I've had to explicitly add back, lest we fail on uses like https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/js/src/jit/shared/Assembler-shared.h#283 (although, that looks suspicious; shouldn't that Register be initialized to Invalid or something?)

Comment 1

[Lars T Hansen [:lth]]

Don't forget the "none" target, I see a few initializations there that might need similar treatment (not 100% sure what's needed but the pattern fits).

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8837425 [details] [diff] [review]
Give Register a constexpr constructor

Review of attachment 8837425 [details] [diff] [review]:
-----------------------------------------------------------------

Just so I understand, though: it's not just WasmTableCallScratchReg, but all the other `static constexpr Register Foo = Bar` in all the Assembler-*.h, right?  It's disappointing that seeing the `constexpr` in the variable declaration isn't a guarantee of this property already.

Comment 3

[Luke Wagner [:luke]]

(FYI Jan)

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8837425 [details] [diff] [review]
Give Register a constexpr constructor

Review of attachment 8837425 [details] [diff] [review]:
-----------------------------------------------------------------

Does that compile on Windows now?
It was not last time I tried.

Comment 5

[(Away)]

(In reply to Lars T Hansen [:lth] from comment #1)
> Don't forget the "none" target, I see a few initializations there that might
> need similar treatment (not 100% sure what's needed but the pattern fits).

Oops -- I did some tree-shuffling yesterday and wound up attaching an old version of this patch. Thanks for catching!

Comment 6

[(Away)]

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 8837425 [details] [diff] [review]
> Give Register a constexpr constructor
> 
> Review of attachment 8837425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just so I understand, though: it's not just WasmTableCallScratchReg, but all
> the other `static constexpr Register Foo = Bar` in all the Assembler-*.h,
> right?  It's disappointing that seeing the `constexpr` in the variable
> declaration isn't a guarantee of this property already.

Right, anything that ultimately derives from some Register foo = { Encoding };

Comment 7

[(Away)]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8837425 [details] [diff] [review]
> Give Register a constexpr constructor
> 
> Review of attachment 8837425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does that compile on Windows now?
> It was not last time I tried.

Seems fine now. When did you try and which part was problematic? The constexpr or the brace-init?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9085a52b43c
Give Register a constexpr constructor. r=luke

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9085a52b43c

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

(In reply to David Major [:dmajor] from comment #7)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > Does that compile on Windows now?
> > It was not last time I tried.
> 
> Seems fine now. When did you try and which part was problematic? The
> constexpr or the brace-init?

Good to know :)
This was the brace-init, at least 2 years ago.

Comment 11

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> (In reply to David Major [:dmajor] from comment #7)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > > Does that compile on Windows now?
> > > It was not last time I tried.
> > 
> > Seems fine now. When did you try and which part was problematic? The
> > constexpr or the brace-init?
> 
> Good to know :)
> This was the brace-init, at least 2 years ago.

To be more precise:

const Register rax = { X86Encoding::rax };  // worked
const Register rax { X86Encoding::rax };    // did not worked