952306 - OdinMonkey: allow module globals to be variable initializers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

To allow:

function f(glob) {
"use asm";
var f32 = glob.Math.fround;
var f0 = f32(0.);
function g() {
  var g0 = f0;
  return +g0;
}
return g;
}

That should lower the number of calls to Math.fround and thus help non asmjs mode.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed implementation plus tests

Drive-by fix: const Float32 globals weren't constructed correctly (using MConstant::New, which infers the kind thanks to the given JS value, which is a double numeric literal, thus incorrectly inferring Double instead of Float).

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8350350 [details] [diff] [review]
proposed implementation plus tests

Cancelling review for now. We can do better for constant Global::Variable by avoiding the load.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Small fix for Float32 constants

This one is pretty trivial, it's just the drive-by fix I was mentioning before.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed implementation and tests

It took me all this time to figure out that the initial patch was already optimizing constant numeric literals and constant imported values. These optimizations are done in FunctionCompiler::loadGlobalVar, which is used in the prepareEmitMIR function. Reading the code again allowed me to make it a little bit cleaner though.

Am I missing other opportunities to optimize that you were thinking at?

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8362613 [details] [diff] [review]
Small fix for Float32 constants

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

::: js/src/jit/AsmJS.cpp
@@ +2252,5 @@
>              return nullptr;
>          if (global.varIsLitConstant()) {
>              JS_ASSERT(global.litConstValue().isNumber());
> +            MDefinition *cst = constant(global.litConstValue(), global.varType().toType());
> +            // constant() already adds it to the block

I think you're fine w/o the comment.  You can just 'return constant(...)' then.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8362613 [details] [diff] [review]
Small fix for Float32 constants

Does this fix need to be uplifted?  That is, will it just result in suboptimal float64 ops or could it cause a MIR type mismatch?

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8362620 [details] [diff] [review]
proposed implementation and tests

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

I think the change we talked about with Alon was that we only allow const variables in local-var initializers (since only these are really "free").  Perhaps you could change the patch to do that?  Should simplify things somewhat.

::: js/src/jit/AsmJS.cpp
@@ +3322,5 @@
>  static bool
> +IsGlobalInitializer(FunctionCompiler &f, ParseNode* initNode, ParseNode *var, const ModuleCompiler::Global **global)
> +{
> +    if (initNode->isKind(PNK_NAME)) {
> +        Rooted<PropertyName *> initName(f.cx(), initNode->name());

Shouldn't need a Rooted here: nothing during asm.js compilation can trigger GC.  That's why everything passes around bare PropertyName*s.

@@ +3368,5 @@
>  
> +    const ModuleCompiler::Global *global = nullptr;
> +    if (IsGlobalInitializer(f, initNode, var, &global)) {
> +        JS_ASSERT(global);
> +        return ExtractGlobalInitializer(f, initNode, var, name, global);

I think it'd be simpler to just inline both functions so it's easy to read the flow of the lookup -> Global -> case analysis.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 8362620 [details] [diff] [review]
> proposed implementation and tests
> 
> Review of attachment 8362620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the change we talked about with Alon was that we only allow const
> variables in local-var initializers (since only these are really "free"). 
> Perhaps you could change the patch to do that?  Should simplify things
> somewhat.
>

Thanks for your quick reviews! I can surely change the patch to do that instead but I thought this patch's goal was to reduce size of generated code in the first place. If we only allow this for const variables, as const isn't generated by default with emscripten (that was true when we discussed the feature, since it was not supported in IE, iirc - it might have changed since), benefits will be very low in general.

On the other hand, if we generate it even for non-const variables, it will create one more MAsmJSLoadGlobalVar in the worst case (=> slightly slower to load the var the first time) *but* greatly reduce the size of generated code (at least one letter for the Math.fround identifier + the pair of parenthesis, for each Float32 initializer).

I can gather data on micro benchmarks to compare size of generated code vs overhead cost if necessary.

Comment 9

[Luke Wagner [:luke]]

Well, the idea is that Emscripten would *start* generating 'const'.  In particular, if your app uses WebGL, then it requires IE11 and IE11 has 'const', so Emscripten would definitely generate it then.  Also, we talked about having a general compatibility parameter to Emscripten that allowed it to assume various things.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Only allow const numeric literals

Alright, it makes things way easier in this case. To avoid to load global variables at all, we also need to prevent const imported variables to be initializers (as they're unknown at compile time). Eventually only const numeric literals can be local variables initializers. Does it work for you?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8362613 [details] [diff] [review]
> Small fix for Float32 constants
> 
> Does this fix need to be uplifted?  That is, will it just result in
> suboptimal float64 ops or could it cause a MIR type mismatch?

This one could easily and should be uplifted (it results in MIR type mismatch and thus correctness issues). I'll land it first on m-i and then ask for the uplift once testing is complete.

Comment 12

[Luke Wagner [:luke]]

Not this patch's fault, but this patch makes me think that we should refactor ModuleCompiler::Global a bit because the case analysis now is just awkward.  In particular, it seems like we should:
 - conflate the Constant case with the Variable-isLitConstant case into: Global::ConstantLiteral (with type/value accessors)
 - split off a ConstantImport so that a Variable is a mutable global variable and ConstantImport is immutable

From a quick scan of how Global is used, this would make all the case analyses simpler and more readable.  What do you think?  If you'd like, you could do that patch in this bug to simplify your patch or, instead, I could review the patch as-is and do the cleanup myself in another bug.  Up to you

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

I'm glad you asked that, as it actually confused me a lot when making this patch. I've found it weird that constant variables declarations were not resulting into Global::Constant. I'll take care of that refactoring.

Comment 14

[Luke Wagner [:luke]]

Thanks!

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Refactoring

Just the refactoring first. I like it, it makes things easier to understand IMO. Not sure on the style (thought about creating a new struct called "constant" in the Global's union, but all fields were repeated from "var", so it didn't seem to be worth it).

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + tests

Less weird looking, indeed.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 8366092 [details] [diff] [review]
Refactoring

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

Thanks!

::: js/src/jit/AsmJS.cpp
@@ +947,2 @@
>                  uint32_t index_;
> +                Value value_;

Could you rename value_ to literalValue_?

@@ +976,2 @@
>          }
> +        Value constValue() const {

Could this be constLiteralValue?

@@ +1308,4 @@
>      void initImportArgumentName(PropertyName *n) { module_->initImportArgumentName(n); }
>      void initBufferArgumentName(PropertyName *n) { module_->initBufferArgumentName(n); }
>  
>      bool addGlobalVarInitConstant(PropertyName *varName, VarType type, const Value &v,

Could you rename this function to addGlobalVarInit()?

@@ +2252,5 @@
>      {
>          if (!curBlock_)
>              return nullptr;
> +
> +        if (global.which() == ModuleCompiler::Global::ConstantLiteral) {

Since the one caller now handles ConstantLiteral separately, could you remove this special case?

@@ +2272,5 @@
>      void storeGlobalVar(const ModuleCompiler::Global &global, MDefinition *v)
>      {
>          if (!curBlock_)
>              return;
> +        unsigned globalDataOffset = module().globalVarIndexToGlobalDataOffset(global.varOrConstIndex());

How about adding JS_ASSERT(!global.isConst())?

@@ +3388,1 @@
>      {

You can also remove the bracing now that the condition is on a single line.

@@ +3604,5 @@
>          if (global->which() != ModuleCompiler::Global::Variable)
>              return f.failName(lhs, "'%s' is not a mutable variable", name);
> +        if (global->which() == ModuleCompiler::Global::ConstantImport)
> +            return f.failName(lhs, "'%s' is a constant import and not mutable", name);
> +        if (global->which() == ModuleCompiler::Global::ConstantLiteral)

These two if's should be unnecessary given the global->which() != Variable test above.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8366094 [details] [diff] [review]
Patch + tests

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

::: js/src/jit/AsmJS.cpp
@@ +3297,5 @@
>  
> +    if (initNode->isKind(PNK_NAME)) {
> +        PropertyName *initName = initNode->name();
> +        if (const ModuleCompiler::Global *global = f.lookupGlobal(initName)) {
> +            switch (global->which()) {

Normally I like a switch b/c it forces you to enumerate all the cases and it gives a compiler warning when a new case is added.  But in this case, since we are inherently limiting ourselves to ConstantLiterals, and that is the case, I'd just use

  if (global->which() !- ModuleCompiler::Global::ConstantLiteral)
    return f.failName...

@@ +3311,5 @@
> +                return f.failName(initNode, "'%s' isn't a possible global variable initializer, needs to be a const numeric literal", initName);
> +            }
> +            MOZ_ASSUME_UNREACHABLE("unknown global kind in variable initializers");
> +        }
> +        return f.failName(initNode, "'%s' needs to be a known global name", initName);

I think you can remove 'known'

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the review.

(In reply to Luke Wagner [:luke] from comment #17)
> 
> @@ +1308,4 @@
> >      void initImportArgumentName(PropertyName *n) { module_->initImportArgumentName(n); }
> >      void initBufferArgumentName(PropertyName *n) { module_->initBufferArgumentName(n); }
> >  
> >      bool addGlobalVarInitConstant(PropertyName *varName, VarType type, const Value &v,
> 
> Could you rename this function to addGlobalVarInit()?

Sure. I've also renamed AsmJSModule::addGlobalVarInitConstant to addGlobalVarInit, to make things consistent.

> 
> @@ +2252,5 @@
> >      {
> >          if (!curBlock_)
> >              return nullptr;
> > +
> > +        if (global.which() == ModuleCompiler::Global::ConstantLiteral) {
> 
> Since the one caller now handles ConstantLiteral separately, could you
> remove this special case?

Indeed. That removes the need of the small fix patch. I'll ask for an uplift of this one, and if there are conflicts, will just propose the small one as a workaround.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Refactoring + test of float32 constant patch

Carrying forward r+

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + test

Carrying forward r+

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3a04b82246
https://hg.mozilla.org/integration/mozilla-inbound/rev/191ba4345c96

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd3a04b82246
https://hg.mozilla.org/mozilla-central/rev/191ba4345c96

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

If I understand correctly the target milestone set in bug 904918 (which is also mozilla29), as one of these patches fixes a misbehavior of bug 904918, I don't need to uplift the fix, is that right Ryan?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Looks like bug 904918 landed after the last uplift, yes.

Comment 26

[Alon Zakai (:azakai)]

This reached stable and I would like to implement emscripten support for it now. However, I'm not sure I'm doing it right - I get

  warning: asm.js type error: 'f0' isn't a possible global variable initializer, needs to be a const numeric literal

on

  var Math_fround=global.Math.fround;
  var f0 = Math_fround(0);

  function func() {
    var f7 = f0; // error happens here
    ..
  }

Is this not the right way to write this?

Comment 27

[Alon Zakai (:azakai)]

The sample in comment #0 also fails for me.

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Alon Zakai (:azakai) from comment #26)
> This reached stable and I would like to implement emscripten support for it
> now. However, I'm not sure I'm doing it right - I get
> 
>   warning: asm.js type error: 'f0' isn't a possible global variable
> initializer, needs to be a const numeric literal
> 
> on
> 
>   var Math_fround=global.Math.fround;
>   var f0 = Math_fround(0);
> 
>   function func() {
>     var f7 = f0; // error happens here
>     ..
>   }
> 
> Is this not the right way to write this?

Comment 7 explains the rational: non-const global variables aren't really free. This patch only implemented support for global initializers which are const variables, thus if you replace 'var' by 'const' in the definition of f0, it will work.

Comment 29

[Alon Zakai (:azakai)]

Ok, thanks, I'll use const then and document that this is IE11+ only.

I see now that f0 is not supported in returns?

 const f0 = f32(0.);
 function f() {
   return f0;
 }

fails to validate on "must be of the form +x, fround(x) or x|0", is that intentional?

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Alon Zakai (:azakai) from comment #29)
> Ok, thanks, I'll use const then and document that this is IE11+ only.
> 
> I see now that f0 is not supported in returns?
> 
>  const f0 = f32(0.);
>  function f() {
>    return f0;
>  }
> 
> fails to validate on "must be of the form +x, fround(x) or x|0", is that
> intentional?

The only way one can return a value without having a coercion is iff the value is a numeric literal (e.g. return 4.2), otherwise any return value needs a coercion. In this case, we need a coercion, like return fround(f0).

Comment 31

[Alon Zakai (:azakai)]

Yes, but my question is why? Couldn't we support  return f0;  as a way to reduce code and speed up non-fround-having JS engines, just like we allow  var x = f0;  now?

Comment 32

[Luke Wagner [:luke]]

Yeah, that makes sense to do.

Comment 33

[Alon Zakai (:azakai)]

Ok, I implemented support in emscripten - when fround is turned on (it's still off by default), we eliminate some fround calls this way.

This helps v8 - in chrome i now see almost no slowdown when using fround on box2d. However, other benchmarks are less clear, so we can't recommend people use fround yet, needs more investigation.

Comment 34

[Alon Zakai (:azakai)]

Actually, I do still see a 2x slowdown in v8. For some reason I'm getting different results on different machines/builds of chrome.