992845 - Add Scalar replacement of new object

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: WIP

Apparently, we want to specialize soonish our ability to remove allocation of non-escaped object without using any generic way of handling this problem (such as Bug 991720).

The current patch (incomplete) provide a dummy escape analysis, and a way to represent the state of an object, as well as merging it to obtain new Phi nodes.  The idea is the same as a precise alias analysis, which is that we want to add Phi nodes which are representing the content of the memory of the new-object.  Each block as an entry BlockState object which (in this case) is an MObjectState recover instruction, which is populated by the predecessors.

Each time a Store is visited, we add another MObjectState.

Each time a Resume Point is visited, we replace the allocation by the MObjectState. (Note: instead of replacing it, we could just add a list of effectful instruction to the resume point, which would make more sense for Bug 992199)

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Add Scalar replacement of objects with simplest escape analysis.

This patch implements an eager version of escape analysis which filters objects which are only manipulated with a known subset of operations.

The main part of this patch implements the scalar replacement bits which emulate the memory of the object at each block entry and adds Phi nodes, almost as we do in IonBuilder.  The memory of the object is not represented in a stack-like format as we do for emulating the stack of the interpreter, but in an MObjectState, which stores slots MDefinitions, and is used to remove the load when encountered.

As we do not yet have a way to list side-effects on resume points, the current way scan all resume points for reference of the MNewObject in order to replace it by the MObjectState.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8417011 [details] [diff] [review]
Add Scalar replacement of objects with simplest escape analysis.

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

Looks good! Nice you did the optimization levels and tracelogging bits.
I will do a full review on Monday. I was just curious, but I don't have the time to look into the IonAnalysis.cpp changes more carefully, right now.

Do we hit this optimization somewhere? Or is this still too simple and only visible in specially crafted benchmarks (which I could understand).

::: js/src/jit/IonAnalysis.cpp
@@ +174,4 @@
>                  inst = block->discardAt(inst);
> +            } else if (!inst->isRecoveredOnBailout() && !inst->hasLiveDefUses() &&
> +                       inst->canRecoverOnBailout())
> +            {

This looks like a bug present in the current implementation. You might want to have this in a different patch, so that if this patch gets backed out, we still have this fix in place?

@@ +2551,5 @@
> +IsObjectEscaped(MInstruction *ins)
> +{
> +    // Check if the object is escaped. If the object is not the first argument
> +    // of either a known Store / Load, then we consider it as escaped. This a
> +    // cheap an conservative escape analysis.

s/an/and/

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8417011 [details] [diff] [review]
Add Scalar replacement of objects with simplest escape analysis.

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

Cool, I have some concerns mentioned below.
Also it feels to me there are a lot of different paths through the code and you only have one test. I have the tendency to think not everything is tested well enough.
So could you add some extra tests testing the different paths that can be taken through the code?

::: js/src/jit-test/tests/ion/recover-objects.js
@@ +1,1 @@
> +setJitCompilerOption("ion.usecount.trigger", 300);

I think you can lower that a lot and still have this working!

@@ +11,5 @@
> +    return { v: (a.v + b.v + c.v + d.v - 10) / 4 };
> +}
> +var uceFault_notSoEmpty = eval(uneval(uceFault).replace('uceFault', 'uceFault_notSoEmpty'));
> +function notSoEmpty() {
> +    for (var i = 0; i < 1000; i++) {

dito

@@ +24,5 @@
> +        a = b = c = d = unused = undefined;
> +    }
> +}
> +
> +notSoEmpty();

Please add a testcase that uses "fun.arguments[0]".
So something like the following, but making sure the scalar replacements works and the right functions are inlined etc.

function test() {
   var a = { test: 1 }
   test2_inlined(a)
}

function test2_inlined(obj) {
   test3_not_inlined();
}

function test3_not_inlined() {
   assertEq(test2_inlined.arguments[0].test, 1);
}

::: js/src/jit/IonAnalysis.cpp
@@ +2563,5 @@
> +
> +        if (uses.index() != 0)
> +            return true;
> +    }
> +

This seems hard to add new checks. I think this is better:

> for (MUseDefIterator uses(ins); uses; uses++) {
>     MDefinition *def = uses.def();
>
>     switch(def->op()) {
>       case MDefinition::Op_StoreFixedSlot:
>       case MDefinition::Op_StoreSlot:
>       case MDefinition::Op_LoadFixedSlot:
>       case MDefinition::Op_LoadFixedSlot:
>         // Not escaped if it is the first argument.
>         if (uses.index() == 0)
>             break;
>         return true;
>       default:
>         return true;
>     }
> }

@@ +2582,5 @@
> +
> +    for (size_t bid = 0; bid < graph.numBlocks(); bid++)
> +        states.infallibleAppend(nullptr);
> +
> +    // Uninitnialized slots have an "undefined" value.

s/Uninitnialized/Uninitialized

@@ +2616,5 @@
> +            }
> +        }
> +
> +
> +        for (MDefinitionIterator ins(*block); ins; ) {

I think you can add here. Since we don't need to iterate the resumepoints for MObjectState.

if (ins->isObjectState()) {
   ins++;
   continue;
}

@@ +2666,5 @@
> +                // If the successor is not dominated then the object cannot flow in
> +                // this basic block without a Phi, but this is guarded by the
> +                // conservative escaped analysis.
> +                if (!objBlock->dominates(succ))
> +                    continue;

The comment explains this should actually be a MOZ_ASSERT(!objBlock->dominates(succ)); ?

@@ +2672,5 @@
> +                if (succ->numPredecessors() > 1) {
> +                    succState = states[succ->id()] = BlockState::Copy(graph.alloc(), state);
> +                    size_t numInputs = succ->numPredecessors();
> +                    for (size_t slot = 0; slot < state->numSlots(); slot++) {
> +                        MPhi *phi = MPhi::New(graph.alloc(), slot /* this is not a stack offset */);

We can't suddenly decide to have random slot() numbers! We might depend on that information! 
We need to abstract this a bit more, so we can have MPhi's that aren't describing a stackslot!
When I thought about improving Alias Analysis this was also needed.

@@ +2694,5 @@
> +            if (succ->numPredecessors() > 1) {
> +                // For each time where the current block appears as a
> +                // predecessor of its successor. (Sadly we have no equivalent of
> +                // MUse for basic blocks, so we might do this operation multiple
> +                // as a block can be enumerated multiple time as a successor)

What about marking a block?
mark/unmark/isMarked?

::: js/src/jit/IonAnalysis.h
@@ +141,5 @@
>  bool
>  AnalyzeArgumentsUsage(JSContext *cx, JSScript *script);
>  
> +bool
> +EagerScalarReplacement(MIRGenerator *mir, MIRGraph &graph);

I think we should create a new file that contains the "Escape analysis" (EscapeAnalysis.h/cpp). Instead of putting this in IonAnalysis. This file will only contains EagerScalarReplacement currently, but eventually do much more.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> Looks good! Nice you did the optimization levels and tracelogging bits.
> I will do a full review on Monday. I was just curious, but I don't have the
> time to look into the IonAnalysis.cpp changes more carefully, right now.
> 
> Do we hit this optimization somewhere? Or is this still too simple and only
> visible in specially crafted benchmarks (which I could understand).

I ran this patch against Bug 1005532 on Octane and I see only 3 occurences of this optimization in TypeScript.

                         Bug 1005532    This patch
            Runs:           1001            1001
           Box2D:          20343           20348        delta: 0.02%
        CodeLoad:          15358           15486        delta: 0.83%
          Crypto:          22614           22633        delta: 0.08%
       DeltaBlue:          27777           27805        delta: 0.10%
     EarleyBoyer:          28044           28073        delta: 0.10%
         Gameboy:          46779           46906        delta: 0.27%
        Mandreel:          23848           23801        delta: -0.20%
 MandreelLatency:          26708           26468        delta: -0.90%
    NavierStokes:          25864           25873        delta: 0.03%
           PdfJS:          15727           15717        delta: -0.06%
        RayTrace:          45275           45404        delta: 0.28%
          RegExp:           2504            2518        delta: 0.56%
        Richards:          20261           20204        delta: -0.28%
           Splay:          15428           15510        delta: 0.53%
    SplayLatency:          10220           10305        delta: 0.83%
      Typescript:          23283           23557        delta: 1.18%
       __total__:          21274           21316        delta: 0.20%
            zlib:          67059           66991        delta: -0.10%

This current approach has 2 main issues:
 - This is made before the Phi removal, so if we do not do like done in the test case we see the local variable being captured in the Phi of the loop and there is not much things we can do.
 - As we do not have a more robust Escape Analysis phase based on the alias analysis, we are unable to optimize when a temporary object is used as argument of another temporary object. (and we would need to remove the writeBarrier too).
 - Type analysis provide lots of poor result.  Fox example, adding any ".v" access in the uce-removed branch prevent this optimization. (because we do not eagerly infer the types of unused/unknown paths?)

I think we can still do a lot with this, such as adding supports for arrays, and maybe re-runing a Phi removal after (if-needed).

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> I see only 3 occurences of this optimization in TypeScript.

I look at one of them, and I guess this is the lineCol temporary structure use in Parser.ptototype.parseComment (typescript-compiler.js:6768) which is removed after the inlining of the 2 functions which are mutating it.

        if(line > 0) {
            lineCol.line = line;
            lineCol.col = (minChar - lineMap[line]);
        }

The interesting fact, is that lineCol.col is not used, so this is a good candidate for RSub :)

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8417011 [details] [diff] [review]
Add Scalar replacement of objects with simplest escape analysis.

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

This is really nice and the algorithm is pretty easy to understand. I'd love to play with this.

Hannes' review had some good points though, feedback+ assuming all comments will be addressed.

::: js/src/jit/IonAnalysis.cpp
@@ +2526,5 @@
> +
> +// Replace properties and allocation of objects which are not escaped by
> +// register and stack allocations.
> +static bool IsObjectEscaped(MInstruction *ins);
> +static bool ScalarReplacementOfObject(MIRGenerator *mir, MIRGraph &graph, MInstruction *ins);

I agree with Hannes that this should be in its own cpp file. Then we could move these functions above the functions that use them, so that we don't need these forward declarations.

@@ +2548,5 @@
> +}
> +
> +bool
> +IsObjectEscaped(MInstruction *ins)
> +{

Nit: MOZ_ASSERT(ins->type() == MIRType_Object);

@@ +2550,5 @@
> +bool
> +IsObjectEscaped(MInstruction *ins)
> +{
> +    // Check if the object is escaped. If the object is not the first argument
> +    // of either a known Store / Load, then we consider it as escaped. This a

Nit: s/This a/This is a

@@ +2552,5 @@
> +{
> +    // Check if the object is escaped. If the object is not the first argument
> +    // of either a known Store / Load, then we consider it as escaped. This a
> +    // cheap an conservative escape analysis.
> +    for (MUseDefIterator uses(ins); uses; uses++) {

I was briefly wondering whether this should be MUseIterator, but avoiding that is the whole point of recover instructions \o/

@@ +2568,5 @@
> +    return false;
> +}
> +
> +typedef MObjectState BlockState;
> +typedef Vector<BlockState *, 0, SystemAllocPolicy> GraphState;

Change this to 4 or 8 or so to avoid growing the vector in most cases.

::: js/src/jit/IonOptimizationLevels.cpp
@@ +53,5 @@
>      // Take normal option values for not specified values.
>      initNormalOptimizationInfo();
>  
>      level_ = Optimization_AsmJS;
> +    esr_ = false;                       // AsmJS has no object.

Nit: s/object/objects

::: js/src/jit/IonOptimizationLevels.h
@@ +63,5 @@
>      // Toggles whether native scripts get inlined.
>      bool inlineNative_;
>  
> +    // Toggles whether eager scalar replacement is used.
> +    bool esr_;

Nit: unlike GVN and LICM, "esr" is not a standard term/optimization AFAIK, so

bool eagerScalarReplacement_;

is a bit longer but also much clearer, especially for people not familiar with the JITs. Also, what does the "eager" refer to, do we have plans to add a lazy scalar replacement? If not I'd simply use scalarReplacement_.

::: js/src/jit/JitOptions.cpp
@@ +36,5 @@
>      // Whether Ion should compile try-catch statements.
>      compileTryCatch = true;
>  
> +    // Toggle whether eager scalar replacement is globally disabled.
> +    disableEsr = false;

We should probably land this disabled-by-default and enable it after a few days/weeks of fuzzing (we should do that with more features).

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Nit: unlike GVN and LICM, "esr" is not a standard term/optimization AFAIK, so
> 
> bool eagerScalarReplacement_;

Or maybe escapeAnalysis_ to make it easier to add similar optimizations?

For instance it'd be great if we could optimize MRegExp to only clone the regexp literal on bailout, maybe we could do that kind of optimization as part of the same "Escape analysis" pass, after ESR.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Jan de Mooij [:jandem] from comment #6)
> > Nit: unlike GVN and LICM, "esr" is not a standard term/optimization AFAIK, so
> > 
> > bool eagerScalarReplacement_;
> 
> Or maybe escapeAnalysis_ to make it easier to add similar optimizations?

Oh yeah. I think this should be escapeAnalysis.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Jan de Mooij [:jandem] from comment #6)
> > Nit: unlike GVN and LICM, "esr" is not a standard term/optimization AFAIK, so
> > 
> > bool eagerScalarReplacement_;

"eager" is here because this escape analysis is dummy and does not play with our alias analysis.  It does things eagerly where we could have a nice escape analysis.

The scalar replacement is supposed to use what comes up of both the alias analysis and the escape analysis, and do transformations based on that.  If we do not do so, we would not be able to handle properly the case where an object is referenced inside an non-escaped one.

var temp = { foo : {} };

> Or maybe escapeAnalysis_ to make it easier to add similar optimizations?

Currently, the *only* escape analysis is restricted to the function which is checking one MNewObject.
I have something more clever that I started last year.

Calling that escape analysis would hurt my feeling as only ~10 lines are really doing the dummy **Analysis**, and the rest is dedicated to the scalar replacement.

(In reply to Jan de Mooij [:jandem] from comment #6)
> This is really nice and the algorithm is pretty easy to understand. I'd love
> to play with this.

I want to abstract this algorithm, because this is not the first time I wrote something similar.  Basically, this algorithm is the same I used for making the precise analysis and the scalar replacement prototype.  And this is exactly why I made a GraphState and BlockState.  If we can extract these, idealy we could transform IonBuilder to emulate the Interpreter stack the same way (and build BlockState - aka ResumePoints).

> @@ +2568,5 @@
> > +    return false;
> > +}
> > +
> > +typedef MObjectState BlockState;
> > +typedef Vector<BlockState *, 0, SystemAllocPolicy> GraphState;
> 
> Change this to 4 or 8 or so to avoid growing the vector in most cases.

This vector would not be growing over time, as it is reserved right at the beginning.

(In reply to Hannes Verschore [:h4writer] from comment #3)
> @@ +2672,5 @@
> > +                if (succ->numPredecessors() > 1) {
> > +                    succState = states[succ->id()] = BlockState::Copy(graph.alloc(), state);
> > +                    size_t numInputs = succ->numPredecessors();
> > +                    for (size_t slot = 0; slot < state->numSlots(); slot++) {
> > +                        MPhi *phi = MPhi::New(graph.alloc(), slot /* this is not a stack offset */);
> 
> We can't suddenly decide to have random slot() numbers! We might depend on
> that information! 
> We need to abstract this a bit more, so we can have MPhi's that aren't
> describing a stackslot!
> When I thought about improving Alias Analysis this was also needed.

I was talking with shu about it this morning, and it is in fact incomplete (incorrect), and the only valid thing is the index of the MDefinition in a MResumePoint operands.  So I will work on the patch to remove MPhi::slot_.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> @@ +2694,5 @@
> > +            if (succ->numPredecessors() > 1) {
> > +                // For each time where the current block appears as a
> > +                // predecessor of its successor. (Sadly we have no equivalent of
> > +                // MUse for basic blocks, so we might do this operation multiple
> > +                // as a block can be enumerated multiple time as a successor)
> 
> What about marking a block?
> mark/unmark/isMarked?

No, the goal here is to fill all the operands of the Phi, and as a basic block might appear multiple times as a predecessor of a block. (such as in the case of a table switch)

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Add Scalar replacement of objects with simplest escape analysis.

Delta: (since attachement 8417011)
 - Move to another ScalarReplacement.{h,cpp} files.
 - Extract replace resume point operands and comment why it is working.
 - Add more test cases.
 - Improve comment above "if (!objBlock->dominates(succ))".
 - Remove "eager" from the name.
 - Replace "esr" by "scalarReplacement".
 - Disable this optimization by default.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8448072 [details] [diff] [review]
Add Scalar replacement of objects with simplest escape analysis.

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

And already a place where this kicks in, now that we have already more Recover instructions implemented?

I'm on pto starting saturday. So request r? from jan if you only manage to address comments after tomorrow.

::: js/src/jit/IonOptimizationLevels.cpp
@@ +27,5 @@
>      edgeCaseAnalysis_ = true;
>      eliminateRedundantChecks_ = true;
>      inlineInterpreted_ = true;
>      inlineNative_ = true;
> +    scalarReplacement_ = true;

These are sorted by alphabet. Can you move this to the end?
(And while you're at it, move autoTruncate_ to the top)

@@ +53,5 @@
>      // Take normal option values for not specified values.
>      initNormalOptimizationInfo();
>  
>      level_ = Optimization_AsmJS;
> +    scalarReplacement_ = false;        // AsmJS has no objects.

same here

::: js/src/jit/IonOptimizationLevels.h
@@ +63,5 @@
>      // Toggles whether native scripts get inlined.
>      bool inlineNative_;
>  
> +    // Toggles whether scalar replacement is used.
> +    bool scalarReplacement_;

same here

@@ +131,5 @@
>      uint32_t usesBeforeCompile(JSScript *script, jsbytecode *pc = nullptr) const;
>  
> +    bool scalarReplacementEnabled() const {
> +        return scalarReplacement_ && !js_JitOptions.disableScalarReplacement;
> +    }

same here

::: js/src/jit/MIR.h
@@ +1845,5 @@
>          return true;
>      }
>  };
>  
> +class MObjectState : public MVariadicInstruction

Add a description about MObjectState.

There is something strange here.
I would assume we have 1 + numFixedSlots() + numSlots() operands.
And have
- getFixedSlot(i) { return getOperand(i+1); } 
- getSlot(i) { return getOperand(i+numFixedSlots()+1); } 

In this current implementation the fixedslots and slots overlap? This can't be right, right? Secondly, why does this work? Can you add a testcase with fixedSlots and normal slots.

@@ +1874,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(ObjectState)
> +
> +    static MObjectState *New(TempAllocator &alloc, MDefinition *obj, MDefinition *undef)

maybe undefinedVal? Since we use "def" a lot, my head was wandering to a strange place to explain undef.

::: js/src/jit/ScalarReplacement.cpp
@@ +96,5 @@
> +    // For each basic block, we record the last/first state of the object in
> +    // each of the basic blocks.
> +    GraphState states;
> +    if (!states.reserve(graph.numBlocks()))
> +        return false;

Since the graph doesn't shrink/increases in size. Lets hoist this reserving of space out this function into the main.
So we only need to reserve this once.

@@ +103,5 @@
> +        states.infallibleAppend(nullptr);
> +
> +    // Uninitialized slots have an "undefined" value.
> +    MBasicBlock *objBlock = obj->block();
> +    MConstant *undef = MConstant::New(graph.alloc(), UndefinedValue());

s/undef/undefinedVal(ue)/

@@ +171,5 @@
> +        for (size_t s = 0; s < block->numSuccessors(); s++) {
> +            MBasicBlock *succ = block->getSuccessor(s);
> +            BlockState *succState = states[succ->id()];
> +
> +            // Eagerly add Phi if the successor block has multiple predecessors.

// When a block has no state yet, create a empty one for the successor

@@ +184,5 @@
> +                    continue;
> +
> +                if (succ->numPredecessors() > 1) {
> +                    succState = states[succ->id()] = BlockState::Copy(graph.alloc(), state);
> +                    size_t numInputs = succ->numPredecessors();

s/numInputs/numPreds

@@ +203,5 @@
> +                } else {
> +                    succState = states[succ->id()] = state;
> +                }
> +            }
> +

// Put the state information of the current block in the successor.

@@ +209,5 @@
> +                // For each time where the current block appears as a
> +                // predecessor of its successor. (Sadly we have no equivalent of
> +                // MUse for basic blocks, so we might do this operation multiple
> +                // time as a block can be enumerated multiple time as a
> +                // predecessor)

I don't understand this comment at all. What are you trying to say here? I think you are saying this is suboptimal. But that we can't do anything about it.
Don't think this comment gives some extra understanding...

@@ +210,5 @@
> +                // predecessor of its successor. (Sadly we have no equivalent of
> +                // MUse for basic blocks, so we might do this operation multiple
> +                // time as a block can be enumerated multiple time as a
> +                // predecessor)
> +                size_t numInputs = succ->numPredecessors();

s/numInputs/numPreds/

@@ +218,5 @@
> +
> +                    for (size_t slot = 0; slot < state->numSlots(); slot++) {
> +                        MPhi *phi = succState->getSlot(slot)->toPhi();
> +                        phi->replaceOperand(p, state->getSlot(slot));
> +                    }

Can you move this for() outside the numPredecessors for loop.
And label the first loop:
// Find position of block in the predecessors list

::: js/src/jit/ScalarReplacement.h
@@ +9,5 @@
> +
> +#ifdef JS_ION
> +
> +// This file declares scalar replacement of objects transformation.
> +#include "jit/IonAllocPolicy.h"

This include is not needed in the ".h" file.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> And already a place where this kicks in, now that we have already more
> Recover instructions implemented?

Non-effectful recover instruction are added way after at DCE time.  So the only thing which might happen is that doing so add more use cases for recover instructions, as we see in the TypeScript compiler.

> ::: js/src/jit/MIR.h
> @@ +1845,5 @@
> >          return true;
> >      }
> >  };
> >  
> > +class MObjectState : public MVariadicInstruction
> 
> Add a description about MObjectState.
> 
> There is something strange here.
> I would assume we have 1 + numFixedSlots() + numSlots() operands.

No, numSlots() is the total number of slots (= fixed + dynamic).
This match what we have on ObjectImpl.

> @@ +218,5 @@
> > +
> > +                    for (size_t slot = 0; slot < state->numSlots(); slot++) {
> > +                        MPhi *phi = succState->getSlot(slot)->toPhi();
> > +                        phi->replaceOperand(p, state->getSlot(slot));
> > +                    }
> 
> Can you move this for() outside the numPredecessors for loop.
> And label the first loop:
> // Find position of block in the predecessors list

No, this what the above comment is stating, that there is no optimal way to do so as one block can have multiple time the same successor.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> (And while you're at it, move autoTruncate_ to the top)

I will make a follow-up patch for that as this is not the only one which is not sorted correctly.

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> (In reply to Hannes Verschore [:h4writer] from comment #12)
> > There is something strange here.
> > I would assume we have 1 + numFixedSlots() + numSlots() operands.
> 
> No, numSlots() is the total number of slots (= fixed + dynamic).
> This match what we have on ObjectImpl.

Oh yeah. I forgot to remove that comment. I was mistakenly thinking you had numFixedSlots and numSlots (and implied numSlots were dynamic slots).

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: Add Scalar replacement of objects with simplest escape analysis.

Delta:
 - Apply Hannes recommendation from comment 12

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8451593 [details] [diff] [review]
Add Scalar replacement of objects with simplest escape analysis.

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

Nice job, the code is pretty easy to follow. r=me with nits addressed.

::: js/src/jit/JitOptions.cpp
@@ +36,5 @@
>      // Whether Ion should compile try-catch statements.
>      compileTryCatch = true;
>  
> +    // Toggle whether eager scalar replacement is globally disabled.
> +    disableScalarReplacement = true; /* experimental */

Nit: // instead of /* */

::: js/src/jit/MIR.h
@@ +1887,5 @@
> +        return res;
> +    }
> +
> +    static MObjectState *Copy(TempAllocator &alloc, MObjectState *state)
> +    {

Maybe move New and Copy to MIR.cpp? MIR.h is huge.

@@ +1916,5 @@
> +        replaceOperand(slot + 1, def);
> +    }
> +
> +    MDefinition *getFixedSlot(uint32_t slot) const {
> +        return getSlot(slot);

Nit: MOZ_ASSERT(slot < numFixedSlots());

@@ +1919,5 @@
> +    MDefinition *getFixedSlot(uint32_t slot) const {
> +        return getSlot(slot);
> +    }
> +    void setFixedSlot(uint32_t slot, MDefinition *def) {
> +        setSlot(slot, def);

And here.

::: js/src/jit/ScalarReplacement.cpp
@@ +41,5 @@
> +        }
> +    }
> +}
> +
> +// Escape simple objects which are not escaped, nor observable by any resume point.

Fix this comment.

@@ +88,5 @@
> +// which emulates the content of the object. Every MLoadFixedSlot and MLoadSlot
> +// is replaced by the corresponding value.
> +//
> +// In order to restore the value of the object correctly in case of bailouts, we
> +// replace all reference of the allocation by the MObjectState definition.

Nit: references, definitions (plural)

@@ +99,5 @@
> +    if (!states.reserve(graph.numBlocks()))
> +        return false;
> +
> +    for (size_t bid = 0; bid < graph.numBlocks(); bid++)
> +        states.infallibleAppend(nullptr);

Instead of the states.reserve(graph.numBlocks()) and infallibleAppend you could do:

if (!states.appendN(nullptr, graph.numBlocks())
    return false;

@@ +107,5 @@
> +    MConstant *undefinedVal = MConstant::New(graph.alloc(), UndefinedValue());
> +    objBlock->insertBefore(obj, undefinedVal);
> +    states[objBlock->id()] = BlockState::New(graph.alloc(), obj, undefinedVal);
> +
> +    // Iterate over each basic block and save the object layout of the object.

Nit: s/object layout of the object/object's layout

@@ +137,5 @@
> +                continue;
> +            }
> +
> +            if (ins->numOperands() != 0 && ins->getOperand(0) == obj) {
> +                if (ins->isLoadFixedSlot()) {

Nit: maybe use "switch (ins->op())" here like you did in IsObjectEscaped, with a |default: MOZ_CRASH("...")|, see bug 1036781.

@@ +172,5 @@
> +            MBasicBlock *succ = block->getSuccessor(s);
> +            BlockState *succState = states[succ->id()];
> +
> +            // When a block has no state yet, create a empty one for the
> +            // successor.

Nit: s/a empty/an empty

@@ +176,5 @@
> +            // successor.
> +            if (!succState) {
> +                // If the successor is not dominated then the object cannot flow
> +                // in this basic block without a Phi.  We know that no Phi exist
> +                // in non-dominated successors as the conservative escaped

Nit: s/exist/exists, s/escaped/escape

@@ +196,5 @@
> +                        // values, and each block later fills the Phi inputs.
> +                        for (size_t p = 0; p < numPreds; p++)
> +                            phi->addInput(undefinedVal);
> +
> +                        // Add Phi in the list of Phi of the basic block.

Nit: s/list of Phi/list of Phis

@@ +235,5 @@
> +
> +bool
> +js::jit::ScalarReplacement(MIRGenerator *mir, MIRGraph &graph)
> +{
> +    GraphState objectStates;

I think this should be in ScalarReplacementOfObject, so that you don't have to pass it to that function and you don't need the states.clear() call.

@@ +250,5 @@
> +    }
> +
> +    return true;
> +}
> +

Nit: remove blank line at the end.

::: js/src/jit/ScalarReplacement.h
@@ +9,5 @@
> +#define jit_ScalarReplacement_h
> +
> +#ifdef JS_ION
> +
> +#include "jit/MIR.h"

You can do this instead to avoid the include:

class MIRGenerator;
class MIRGraph;

(see also LICM.h)

::: js/src/shell/js.cpp
@@ +6272,5 @@
>          || !op.addBoolOption('\0', "no-ion", "Disable IonMonkey")
>          || !op.addBoolOption('\0', "no-asmjs", "Disable asm.js compilation")
>          || !op.addBoolOption('\0', "no-native-regexp", "Disable native regexp compilation")
> +        || !op.addStringOption('\0', "ion-scalar-replacement", "on/off",
> +                               "Scalar Replacement (default: on, off to disable)")

disableScalarReplacement is true so "(default: off, on to enable)"

Comment 18

[Nicolas B. Pierron [:nbp]]

As discuss on IRC with jandem, this patch does not contain the specialization for dynamic slots.
I opened Bug 1039607 to handle dynamic slots and the MSlots instruction.

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=24177f64eea0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a822934b97

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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