1052514 - OdinMonkey: Refactor NumLit

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

This bug moves NumLit into AsmJSModule.h, renames it to AsmJSNumLit, to help generalizing for SIMD literals in the future.

A few explanations:
- The methods toVarType() and type() (which was a conversion to AsmJSValidate.cpp's Type) have been changed as constructors of the related types: VarType(const AsmJSNumLit &lit), Type(const AsmJSNumLit &lit).
- Type's Which enum is defined after AsmJSNumLit's Which enum (things were the other way around before).
- To be able to embed AsmJSNumLit into an union, it needs no user-defined ctor. For that purpose, constructors have been removed and a static Create function has been introduced.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Move AsmJSNumLit and use it in Globals

A first patch. More can be done for AsmJSModule's Globals.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8471637 [details] [diff] [review]
Move AsmJSNumLit and use it in Globals

Canceling r? for now, as we can probably do better (i.e. removing the RetType parameter from CheckMathBuiltinCall).

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1052514-1.patch

How silly of me -- I cancelled the review on the wrong patch before leaving on PTO :/

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1052514-2.patch

This second patch adds a use of AsmJSNumLit in AsmJSModule.h as well. Hopefully shouldn't change too much when adding SIMD numlits in the SIMD patch, but just in case, not setting r? for now.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8478367 [details] [diff] [review]
bug1052514-1.patch

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

Great!

::: js/src/asmjs/AsmJSModule.h
@@ +79,5 @@
>  };
>  
> +// Represents the type and value of an asm.js numeric literal.
> +//
> +// A literal is a double iff the literal contains an exponent or decimal point

Pre-existing: this is long out of date; can you s/an exponent or/a/.

@@ +105,5 @@
> +    Value value_;
> +
> +  public:
> +    static AsmJSNumLit Create(Which w, Value v)
> +    {

Can you put the { at the end of the preceding line?

::: js/src/asmjs/AsmJSValidate.cpp
@@ +399,5 @@
>      Which which_;
>  
>    public:
>      Type() : which_(Which(-1)) {}
> +    Type(const AsmJSNumLit &lit) {

For both Type and VarType, how about a static member function 'Of' so you could write VarType/Type::Of(literal)?

@@ +913,5 @@
>          union {
>              struct {
>                  VarType::Which type_;
>                  uint32_t index_;
> +                AsmJSNumLit literalValue_;

Can we remove type_ now, since I believe literalValue_ determines the type.

@@ +1318,3 @@
>          uint32_t index;
> +        VarType type = VarType(lit);
> +        if (!module_->addGlobalVarInit(lit.value(), type.toCoercion(), &index))

With AsmJSNumLit, you could change AsmJSModule::Global::pod.u.var to be:

 struct {
   uint32_t index_;
   VarInitKind initKind_;
   union {
     AsmJSNumLit initLiteral_;
     AsmJSCoercion initCoercion_;
 } var;

which expresses that either we are initing with a literal or by coercing an imported value.  With that change, you could just pass 'lit' here; no coercion argument.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1052514-2.patch

This finishes the second patch and implements your last comment.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8479097 [details] [diff] [review]
bug1052514-2.patch

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

Great!

::: js/src/asmjs/AsmJSModule.h
@@ +171,3 @@
>                      union {
> +                        AsmJSCoercion coercion_;
> +                        AsmJSNumLit constant_; // will only contain int32/double

I think the comment is no longer necessary (it was mostly to point out that we didn't need to trace the Value, but that now seems obvious for a numeric literal).

@@ +216,3 @@
>              JS_ASSERT(pod.which_ == Variable);
>              JS_ASSERT(pod.u.var.initKind_ == InitConstant);
> +            return pod.u.var.u.constant_;

Perhaps rename 'constant_' to 'numLit_' for symmetry with method renaming?

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1052514-1.patch

addressed reviewer's feedback

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1052514-2.patch

addressed reviewer's feedback

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/30524bc6f4cf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f44966d998

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Backout for jit test failures on Linux x64 Opt, asm.js/testHeapAccess:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a576a4743bec

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Fix

fwiw, the fix was to change varOrConstValue to return Value instead of const Value&. Strangely, it worked on debug build but not on opt builds.
Tested on x64 debug/opt, x86 debug/opt and it looks good locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/badda824a0b7

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30524bc6f4cf
https://hg.mozilla.org/mozilla-central/rev/badda824a0b7