852791 - IonMonkey: Improve constant propagation on string concatenation.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

I noticed while looking at iongraph output[1] (can also be viewed with the JIT Inspector) that we are not doing any compile time optimizations on string concatenation following a non-constant string concatenation.  The following code

  var s = "f" + "oo" + x + "b" + "ar";

should produce the same code as:

  var s = "foo" + x + "bar";

where both "foo" and "bar" constants are folded.

This optimization consist mostly of adding constant folding to MConcat, and handle the case of a sequence of MConcat with right hand side constants, and to fold them into one MConstant.

All modifications are likely to be done in js/src/ion/MIR.cpp, but it would be best to understand how the foldsTo functions are called / used and working before making any modifications.

[1] Getting the output of this tool is described in the Hacking tips page of SpiderMonkey.  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_%28JS_shell%29

Comment 1

[Govind kumar]

Sir
I want to handle this bug,it will be great opportunity for me to work on this bug.i am well equipped with the knowledge of c/c++.

Comment 2

[Till Schneidereit [:till]]

Great!

Ask here or on IRC in the #jsapi channel (Nicolas is "nbp" on IRC, but others will gladly help you, too) if you have any additional questions. Feel free to ask as many question as you want!

Comment 3

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

On 04/04/2013 11:51 AM, 井関正也 wrote:
> 2013年4月1日月曜日 23時32分28秒 UTC+9 井関正也:
>> I'm interested in IonMonkey.
>>
>> I read some code in js/src/ion.But I can't understand how IonMonkey analyze the code.
>>
>> When type javascript code in console or commnad line , What function call firstly?
>
> Oh ! I understand.
> Thanks.
>
> I make member function
> MDefinition *
> MConcat::foldsTo(bool useValueNumbers)
> {
>      if ((type() == MIRType_String) && (rhs()->isConstant()))
>      {
>          if((type() == MIRType_String) && (lhs()->isConstant()))
>          {
>              return MConstant::New(MergeStrValue);
>          }
>          else
>          {
>              return MConstant::New(rhs()->toConstant()->value());
>          }
>      }
>      return this;
> }
> I'd like to merger JSString. I think I use js::ConcatStrings. But It doesn't have JSContext in this scope.
> there are other way?

Hum …

the JSContext* has not been added here because this code can run in a parallel thread when we are compiling, and we should not add a JSContext here.

The only case where we can allocate these GC object (new strings) is in the link phase.  So we need to store create a new MConstant MIR node which wrap a buffer containing the content of the new string which would be allocated in the link phase (CodeGenerator::link).

To do that you will have to add an index into the CodeGeneratorShared class which will refer to locations which have to be patched during the link phase, such as the constant index in the constant pool (see CodeGeneratorShared::encodeSlots) can be updated after we have completed the construction of the assembly.

tl;dr:
 - Create a new type similar to MConstant. (MIR.h, MOpcode.h)
 - Add it to the lowering. (LIR-Common.h & Lowering.cpp & LOpcode.h)
 - Add a CodeGenerator function for it. (CodeGenerator.cpp)
 - Allocate a slot in the snapshot for this new MIR type. (CodeGeneratorShared::encodeSlots)
 - Iterate on the vector of patch offset to allocate the new strings (CodeGenerator::link)

Comment 4

[Masaya Iseki[:isk](UTC+9)]

Sorry,I can't understand what I need to do.

I make new class like MConstant in (MIR.h)
class MStringConstant : public MNullaryInstruction
{
    Value value_;

  protected:
    MStringConstant(const Value &v);

  public:
    INSTRUCTION_HEADER(Constant)
    static MStringConstant *New(const Value &v);

    const js::Value &value() const {
        return value_;
    }
    const js::Value *vp() const {
        return &value_;
    }

    void printOpcode(FILE *fp);

    HashNumber valueHash() const;
    bool congruentTo(MDefinition * const &ins) const;

    AliasSet getAliasSet() const {
        return AliasSet::None();
    }

    void computeRange();
    bool truncate();
};  

What add to LIR-Common.h & Lowering.cpp & LOpcod.h?

Comment 5

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

(In reply to iseki.m.aa from comment #4)
> Sorry,I can't understand what I need to do.
> 
> I make new class like MConstant in (MIR.h)
> class MStringConstant : public MNullaryInstruction
> {
>     Value value_;
> 
>   protected:
>     MStringConstant(const Value &v);
> 
>   public:
>     INSTRUCTION_HEADER(Constant)
>     static MStringConstant *New(const Value &v);
> 
>     const js::Value &value() const {
>         return value_;
>     }
>     const js::Value *vp() const {
>         return &value_;
>     }
> 
>     void printOpcode(FILE *fp);
> 
>     HashNumber valueHash() const;
>     bool congruentTo(MDefinition * const &ins) const;
> 
>     AliasSet getAliasSet() const {
>         return AliasSet::None();
>     }
> 
>     void computeRange();
>     bool truncate();
> };  
> 
> What add to LIR-Common.h & Lowering.cpp & LOpcod.h?

If you compile it the compiler should report to you that you are missing declarations / definitions of functions.

Look at how MConstant is used, you will understand the flow of values inside the compiler.

http://dxr.mozilla.org/search?tree=mozilla-central&q=%2Btype-ref%3Ajs%3A%3Aion%3A%3AMConstant

You should not need the computeRange and truncate methods are they are only used by the RangeAnalysis which only deals with numerical values.

You need to first make this work for string constants, without any constant folding.  And try to replace a few MConstant which are working with Strings by this MStringConstant.  Then as soon as this compile and pass the test suite, you can already upload a WIP (work-in-progress) patch to bugzilla.  Then you can think of making the constant folding of strings …

Comment 6

[Masaya Iseki[:isk](UTC+9)]

I change MStringConstants( in MIR.h ) below:


class MStringConstant : public MNullaryInstruction
{
    Value value_;

  protected:
    MStringConstant(const Value &v);

  public:
    INSTRUCTION_HEADER(StringConstant)
    static MStringConstant *New(const Value &v);

    const js::Value &value() const {
        return value_;
    }
    const js::Value *vp() const {
        return &value_;
    }

    void printOpcode(FILE *fp);

    HashNumber valueHash() const;
    bool congruentTo(MDefinition * const &ins) const;

    AliasSet getAliasSet() const {
        return AliasSet::None();
    }

};

and I add StringConstant to MIR_OPCODE_LIST (in MOpcodes.h):

#define MIR_OPCODE_LIST(_)                                  \
    _(Constant)                                                             \
    _(StringConstant)                                                  \
    _(Parameter)                                                           \
....

But I can,t compile it. This code generate 1 error.

/mozilla-central/js/src/ion/ParallelArrayAnalysis.cpp:353:26: error: variable type 'ParallelArrayVisitor'
      is an abstract class
    ParallelArrayVisitor visitor(cx_, graph);
                         ^
/mozilla-central/js/src/ion/MOpcodes.h:192:21: note: unimplemented pure virtual method
      'visitStringConstant' in 'ParallelArrayVisitor'
    MIR_OPCODE_LIST(VISIT_INS)
                    ^
/mozilla-central/js/src/ion/MOpcodes.h:17:5: note: expanded from macro 'MIR_OPCODE_LIST'
    _(StringConstant)                                                       \
    ^
/mozilla-central/js/src/ion/MOpcodes.h:191:36: note: expanded from macro 'VISIT_INS'
#define VISIT_INS(op) virtual bool visit##op(M##op *) = 0;
                                   ^
<scratch space>:19:1: note: expanded from macro 'visit'
visitStringConstant
^
1 error generated.
make[1]: *** [ParallelArrayAnalysis.o] Error 1
make: *** [default] Error 2

What should I do to build.

Comment 7

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

(In reply to iseki.m.aa from comment #6)
> and I add StringConstant to MIR_OPCODE_LIST (in MOpcodes.h):
> 
> #define MIR_OPCODE_LIST(_)                                  \
>     _(Constant)                                                             \
>     _(StringConstant)                                                  \
>     _(Parameter)                                                           \
> ....
> 
> But I can,t compile it. This code generate 1 error.
> 
> /mozilla-central/js/src/ion/ParallelArrayAnalysis.cpp:353:26: error:
> variable type 'ParallelArrayVisitor'
>       is an abstract class
>     ParallelArrayVisitor visitor(cx_, graph);
>                          ^
> /mozilla-central/js/src/ion/MOpcodes.h:192:21: note: unimplemented pure
> virtual method
>       'visitStringConstant' in 'ParallelArrayVisitor'
>     MIR_OPCODE_LIST(VISIT_INS)
>                     ^
> /mozilla-central/js/src/ion/MOpcodes.h:17:5: note: expanded from macro
> 'MIR_OPCODE_LIST'
>     _(StringConstant)                                                       \
>     ^

Parallel Array code add a visitor which defines if instructions are safe to be executed in parallel execution mode.  In general, if you have any doubt, it is safe to use the UNSAFE_OP.  In the case of MStringConstant it would be safe to do it in parallel execution, as the allocation happen at the link phase, and this value is later seen as a constant.

Comment 8

[Masaya Iseki[:isk](UTC+9)]

Thanks
I add the UNSAFE_OP in ParallelArrayAnalysis.cpp.

I found MConstant which with working with String. This is in IonBuilder::pushConstant (in IonBuilder.cpp).
I change this method below:
bool
IonBuilder::pushConstant(const Value &v)
{
    if (v.isString())
    {
        MStringConstant *ins = MStringConstant::New(v);
        current->add(ins);
        current->push(ins);
    }
    else {
        MConstant *ins = MConstant::New(v);
        current->add(ins);
        current->push(ins);
    }
    return true;
}

When I compile it, I have error message.

Undefined symbols for architecture x86_64:
  "js::ion::MStringConstant::New(JS::Value const&)", referenced from:
      js::ion::IonBuilder::inspectOpcode(JSOp) in IonBuilder.o
      js::ion::IonBuilder::jsop_getgname(JS::Handle<js::PropertyName*>) in IonBuilder.o
      js::ion::IonBuilder::jsop_setprop(JS::Handle<js::PropertyName*>) in IonBuilder.o
      js::ion::IonBuilder::getPropTryCommonGetter(bool*, JS::Handle<jsid>, js::types::StackTypeSet*, js::types::StackTypeSet*, js::ion::TypeOracle::UnaryTypes) in IonBuilder.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [libmozjs.dylib] Error 1
make: *** [default] Error 2

Comment 9

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

(In reply to iseki.m.aa from comment #8)
> bool
> IonBuilder::pushConstant(const Value &v)
> {
>     if (v.isString())
>     {

Little style detail:

  if (… one line …) {
    … multiple lines …
  } else {
    … multiple lines …
  }


> When I compile it, I have error message.
> 
> Undefined symbols for architecture x86_64:
>   "js::ion::MStringConstant::New(JS::Value const&)", referenced from:
>       …

You don't have to come back on this bug at each compilation error, Google knows a lot about error messages too ;)

Where is the static function MStringConstant::New defined?

Comment 10

[Masaya Iseki[:isk](UTC+9)]

I didn't define MStringConstant::New yet. Thanks.

Now I can call below case in CodeGeneratorShared::encodeSlots.

          case MIRType_ConstantString:
          {
            printf("constant\n");
            MConstant *constant = mir->toConstant();
            uint32_t index;
            if (!graph.addConstantToPool(constant->value(), &index))
                return false;
            snapshots_.addConstantPoolSlot(index);
            break;
          }

I can compile it.
When I type  " var s = 'ga' ", console display Assertion failure: isConstant(), at ./ion/MIR.h:7320.
I have to replace a isConstant() with isStringConstant (I add StringConstant to MOpcode.h).
But there are many isConstant(). I don't know what should be replaced.

Comment 11

[Masaya Iseki[:isk](UTC+9)]

Attachment: work-in-progress

Comment 12

[Masaya Iseki[:isk](UTC+9)]

Attachment: work-in-progress

Comment 13

[Masaya Iseki[:isk](UTC+9)]

Comment on attachment 743026 [details] [diff] [review]
work-in-progress

To store MStringConstant, I add 
MStringConstant::New(rhs()->toConstant()->value());
in MConcat::foldsTo(bool useValueNumbers)

But this constant is discarded in
ValueNumberer::computeValueNumbers() 
...
                if (ins != *iter) {
                    iter = block->discardDefAt(iter);
                    continue;
                }

Because of it, I can't store MStringConstant.
If I can store MStringConstant until CodeGenerator::link(), how to delete MStringConstant in constantPool?

Comment 14

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

Comment on attachment 743026 [details] [diff] [review]
work-in-progress

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

::: js/public/Value.h
@@ +75,4 @@
>      JSVAL_TYPE_STRING              = 0x05,
>      JSVAL_TYPE_NULL                = 0x06,
>      JSVAL_TYPE_OBJECT              = 0x07,
> +    JSVAL_TYPE_STRING_CONSTANT     = 0x08,

Why adding a new type? this is unnecessary and would add more complexity.

::: js/src/ion/IonTypes.h
@@ +58,4 @@
>      MIRType_Int32,
>      MIRType_Double,
>      MIRType_String,
> +    MIRType_StringConstant,

A constant string is a constant, you don't need another MIRType for that.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +180,4 @@
>              }
>              break;
>            }
> +          case MIRType_StringConstant:

instead of having a check for a special MIRType you can check under MIRType_String:

  if (mir->isStringConstant()) {
      …
  }

@@ +184,5 @@
> +          {
> +            MStringConstant *constant = mir->toStringConstant();
> +            constant->value().toString()->dump();
> +            uint32_t index;
> +            if (!graph.addConstantToPool(constant->value(), &index))

This is almost the idea except that instead of adding a value inside it you want to add an index which refers to the concatenation that needs to be done at the link phase, as you need to allocate GC memory and we cannot else-where.

Comment 15

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

(In reply to iseki.m.aa from comment #13)
> To store MStringConstant, I add 
> MStringConstant::New(rhs()->toConstant()->value());
> in MConcat::foldsTo(bool useValueNumbers)

In MConcat::foldsTo, you probably want to have a MStringConstant which is constructed that way:

   MStringConstant::New(lhs()->toConstant()->value(), rhs()->toConstant()->value());

As you are doing a concatenation.  Also, you might also note that this sounds just like adding a flag on MConcat.

> But this constant is discarded in
> ValueNumberer::computeValueNumbers() 
> ...
>                 if (ins != *iter) {
>                     iter = block->discardDefAt(iter);
>                     continue;
>                 }
> 
> Because of it, I can't store MStringConstant.

Probably because the result is not used or returned.  That's nice that you put a patch on bugzilla but it is incomplete.

> If I can store MStringConstant until CodeGenerator::link(), how to delete
> MStringConstant in constantPool?

The MStringConstant stored in the constant pool will be substituted by the concatenation of the strings contains in the MStringConstant.

Comment 16

[Masaya Iseki[:isk](UTC+9)]

Attachment: work-in-progress

Comment 17

[Masaya Iseki[:isk](UTC+9)]

Comment on attachment 754131 [details] [diff] [review]
work-in-progress

>diff --git a/js/public/Value.h b/js/public/Value.h
>index a9a7bfd..c9a1ecb 100644
>--- a/js/public/Value.h
>+++ b/js/public/Value.h
>@@ -1433,6 +1433,7 @@ class ValueOperations
>     bool isInt32() const { return value()->isInt32(); }
>     bool isDouble() const { return value()->isDouble(); }
>     bool isString() const { return value()->isString(); }
>+    bool isStringConstant() const { return value()->isStringConstant(); }
>     bool isObject() const { return value()->isObject(); }
>     bool isMagic() const { return value()->isMagic(); }
>     bool isMagic(JSWhyMagic why) const { return value()->isMagic(why); }
>diff --git a/js/src/ion/CodeGenerator.cpp b/js/src/ion/CodeGenerator.cpp
>index 8b40d06..d53f1b4 100644
>--- a/js/src/ion/CodeGenerator.cpp
>+++ b/js/src/ion/CodeGenerator.cpp
>@@ -887,6 +887,17 @@ CodeGenerator::visitPointer(LPointer *lir)
> }
>
> bool
>+CodeGenerator::visitStringPointer(LStringPointer *lir)
>+{
>+    if (lir->kind() == LStringPointer::GC_THING) {
>+        masm.movePtr(ImmGCPtr(lir->gclptr()), ToRegister(lir->output()));
>+    } else {
>+        masm.movePtr(ImmWord(lir->lptr()), ToRegister(lir->output()));
>+    }
>+    return true;
>+}
>+
>+bool
> CodeGenerator::visitSlots(LSlots *lir)
> {
>     Address slots(ToRegister(lir->object()), JSObject::offsetOfSlots());
>diff --git a/js/src/ion/CodeGenerator.h b/js/src/ion/CodeGenerator.h
>index 263b859..809207c 100644
>--- a/js/src/ion/CodeGenerator.h
>+++ b/js/src/ion/CodeGenerator.h
>@@ -81,6 +81,7 @@ class CodeGenerator : public CodeGeneratorSpecific
>     bool visitLambdaForSingleton(LLambdaForSingleton *lir);
>     bool visitParLambda(LParLambda *lir);
>     bool visitPointer(LPointer *lir);
>+    bool visitStringPointer(LStringPointer *lir);
>     bool visitSlots(LSlots *lir);
>     bool visitStoreSlotV(LStoreSlotV *store);
>     bool visitElements(LElements *lir);
>diff --git a/js/src/ion/LIR-Common.h b/js/src/ion/LIR-Common.h
>index c634e17..6aebb55 100644
>--- a/js/src/ion/LIR-Common.h
>+++ b/js/src/ion/LIR-Common.h
>@@ -174,6 +174,51 @@ class LPointer : public LInstructionHelper<1, 0, 0>
>     }
> };
>
>+// StringConstant pointer.
>+class LStringPointer : public LInstructionHelper<1, 0, 0>
>+{
>+  public:
>+    enum Kind {
>+        GC_THING,
>+        NON_GC_THING
>+    };
>+
>+  private:
>+    void *lptr_;
>+    void *rptr_;
>+    Kind kind_;
>+
>+  public:
>+    LIR_HEADER(StringPointer)
>+
>+    LStringPointer(gc::Cell *lptr,gc::Cell *rptr)
>+      : lptr_(lptr), rptr_(rptr), kind_(GC_THING)
>+    { }
>+
>+    LStringPointer(void *lptr, void *rptr, Kind kind)
>+      : lptr_(lptr), rptr_(rptr), kind_(kind)
>+    { }
>+
>+    void *lptr() const {
>+        return lptr_;
>+    }
>+    void *rptr() const {
>+        return rptr_;
>+    }
>+    Kind kind() const {
>+        return kind_;
>+    }
>+
>+    gc::Cell *gclptr() const {
>+        JS_ASSERT(kind() == GC_THING);
>+        return (gc::Cell *) lptr_;
>+    }
>+    gc::Cell *gcrptr() const {
>+        JS_ASSERT(kind() == GC_THING);
>+        return (gc::Cell *) rptr_;
>+    }
>+};
>+
> // A constant Value.
> class LValue : public LInstructionHelper<BOX_PIECES, 0, 0>
> {
>diff --git a/js/src/ion/LOpcodes.h b/js/src/ion/LOpcodes.h
>index 5f2f50e..9bbcd5c 100644
>--- a/js/src/ion/LOpcodes.h
>+++ b/js/src/ion/LOpcodes.h
>@@ -15,6 +15,7 @@
>     _(MoveGroup)                    \
>     _(Integer)                      \
>     _(Pointer)                      \
>+    _(StringPointer)                \
>     _(Double)                       \
>     _(Value)                        \
>     _(Parameter)                    \
>diff --git a/js/src/ion/MIR.cpp b/js/src/ion/MIR.cpp
>index 8eb9b11..21fb652 100644
>--- a/js/src/ion/MIR.cpp
>+++ b/js/src/ion/MIR.cpp
>@@ -402,6 +402,42 @@ MConstantElements::printOpcode(FILE *fp)
>     fprintf(fp, " %p", value());
> }
>
>+MStringConstant *
>+MStringConstant::New(const Value &lv,const Value &rv)
>+{
>+    return new MStringConstant(lv,rv);
>+}
>+
>+MStringConstant::MStringConstant(const js::Value &lvp,const js::Value &rvp)
>+  : leftValue_(lvp),rightValue_(rvp)
>+{
>+    setResultType(MIRType_String);
>+    setMovable();
>+}
>+
>+HashNumber
>+MStringConstant::valueHash() const
>+{
>+    // This disregards some state, since values are 64 bits. But for a hash,
>+    // it's completely acceptable.
>+    return (HashNumber)JSVAL_TO_IMPL(leftValue_).asBits;
>+}
>+
>+bool
>+MStringConstant::congruentTo(MDefinition * const &ins) const
>+{
>+    if (!ins->isStringConstant())
>+        return false;
>+    return ((ins->toStringConstant()->leftValue() == leftValue()) && (ins->toStringConstant()->rightValue() == rightValue()));
>+}
>+
>+void
>+MStringConstant::printOpcode(FILE *fp)
>+{
>+    PrintOpcodeName(fp, op());
>+    fprintf(fp, "string %p,string %p", (void *)leftValue().toString(),(void *)rightValue().toString());
>+}
>+
> MParameter *
> MParameter::New(int32_t index, const types::StackTypeSet *types)
> {
>@@ -2047,3 +2083,18 @@ MAsmJSCall::New(Callee callee, const Args &args, MIRType resultType, size_t spIn
>     return call;
> }
>
>+MDefinition *
>+MConcat::foldsTo(bool useValueNumbers)
>+{
>+    if ((type() == MIRType_String))
>+    {
>+        if((lhs()->isConstant()) && (rhs()->isConstant()))
>+        {
>+            printf("concat\n");
>+            lhs()->toConstant()->value().toString()->dump();
>+            rhs()->toConstant()->value().toString()->dump();
>+            return MStringConstant::New(lhs()->toConstant()->value(),rhs()->toConstant()->value());
>+        }
>+    }
>+    return this;
>+}
>diff --git a/js/src/ion/MIR.h b/js/src/ion/MIR.h
>index bcc4e74..4c34b6b 100644
>--- a/js/src/ion/MIR.h
>+++ b/js/src/ion/MIR.h
>@@ -714,6 +714,42 @@ class MConstant : public MNullaryInstruction
>     bool truncate();
> };
>
>+class MStringConstant : public MNullaryInstruction
>+{
>+    Value leftValue_;
>+    Value rightValue_;
>+
>+  protected:
>+    MStringConstant(const Value &lv,const Value &rv);
>+
>+  public:
>+    INSTRUCTION_HEADER(StringConstant)
>+    static MStringConstant *New(const Value &lv,const Value &rv);
>+
>+    const js::Value &leftValue() const {
>+        return leftValue_;
>+    }
>+    const js::Value &rightValue() const {
>+        return rightValue_;
>+    }
>+    const js::Value *leftVp() const {
>+        return &leftValue_;
>+    }
>+    const js::Value *rightVp() const {
>+        return &rightValue_;
>+    }
>+
>+    void printOpcode(FILE *fp);
>+
>+    HashNumber valueHash() const;
>+    bool congruentTo(MDefinition * const &ins) const;
>+
>+    AliasSet getAliasSet() const {
>+        return AliasSet::None();
>+    }
>+
>+};
>+
> class MParameter : public MNullaryInstruction
> {
>     int32_t index_;
>@@ -3215,6 +3251,7 @@ class MConcat
>     AliasSet getAliasSet() const {
>         return AliasSet::None();
>     }
>+    MDefinition *foldsTo(bool useValueNumbers);
> };
>
> class MCharCodeAt
>diff --git a/js/src/ion/MOpcodes.h b/js/src/ion/MOpcodes.h
>index aa72199..a0da729 100644
>--- a/js/src/ion/MOpcodes.h
>+++ b/js/src/ion/MOpcodes.h
>@@ -14,6 +14,7 @@ namespace ion {
>
> #define MIR_OPCODE_LIST(_)                                                  \
>     _(Constant)                                                             \
>+    _(StringConstant)                                                       \
>     _(Parameter)                                                            \
>     _(Callee)                                                               \
>     _(TableSwitch)                                                          \
>diff --git a/js/src/ion/ParallelArrayAnalysis.cpp b/js/src/ion/ParallelArrayAnalysis.cpp
>index f3f0277..0ce5183 100644
>--- a/js/src/ion/ParallelArrayAnalysis.cpp
>+++ b/js/src/ion/ParallelArrayAnalysis.cpp
>@@ -119,6 +119,7 @@ class ParallelArrayVisitor : public MInstructionVisitor
>     // obviously safe for now.  We can loosen as we need.
>
>     SAFE_OP(Constant)
>+    UNSAFE_OP(StringConstant)
>     SAFE_OP(Parameter)
>     SAFE_OP(Callee)
>     SAFE_OP(TableSwitch)
>diff --git a/js/src/ion/arm/Lowering-arm.cpp b/js/src/ion/arm/Lowering-arm.cpp
>index 25854c9..9a39d91 100644
>--- a/js/src/ion/arm/Lowering-arm.cpp
>+++ b/js/src/ion/arm/Lowering-arm.cpp
>@@ -69,6 +69,16 @@ LIRGeneratorARM::visitConstant(MConstant *ins)
> }
>
> bool
>+LIRGeneratorARM::visitStringConstant(MStringConstant *ins)
>+{
>+    // Emit non-double constants at their uses.
>+    if (ins->canEmitAtUses())
>+        return emitAtUses(ins);
>+
>+    return LIRGeneratorShared::visitStringConstant(ins);
>+}
>+
>+bool
> LIRGeneratorARM::visitBox(MBox *box)
> {
>     MDefinition *inner = box->getOperand(0);
>diff --git a/js/src/ion/arm/Lowering-arm.h b/js/src/ion/arm/Lowering-arm.h
>index 6d13b2b..72472ff 100644
>--- a/js/src/ion/arm/Lowering-arm.h
>+++ b/js/src/ion/arm/Lowering-arm.h
>@@ -55,6 +55,7 @@ class LIRGeneratorARM : public LIRGeneratorShared
>
>   public:
>     bool visitConstant(MConstant *ins);
>+    bool visitStringConstant(MStringConstant *ins);
>     bool visitBox(MBox *box);
>     bool visitUnbox(MUnbox *unbox);
>     bool visitReturn(MReturn *ret);
>diff --git a/js/src/ion/shared/Assembler-shared.h b/js/src/ion/shared/Assembler-shared.h
>index 37b6fc8..c4e471a 100644
>--- a/js/src/ion/shared/Assembler-shared.h
>+++ b/js/src/ion/shared/Assembler-shared.h
>@@ -112,6 +112,27 @@ struct ImmWord
>         return reinterpret_cast<void *>(value);
>     }
> };
>+// Pointer-sized immediate for StringPointer.
>+struct ImmStringWord
>+{
>+    uintptr_t leftValue;
>+    uintptr_t rightValue;
>+
>+    explicit ImmStringWord(uintptr_t lValue,uintptr_t rValue) : leftValue(lValue), rightValue(rValue)
>+    { }
>+    explicit ImmStringWord(const void *lptr,const void *rptr) : leftValue(reinterpret_cast<uintptr_t>(lptr)), rightValue(reinterpret_cast<uintptr_t>(rptr))
>+    { }
>+
>+    // Note - this constructor is not implemented as it should not be used.
>+    explicit ImmStringWord(gc::Cell *cell);
>+
>+    void *asLeftPointer() {
>+        return reinterpret_cast<void *>(leftValue);
>+    }
>+    void *asRightPointer() {
>+        return reinterpret_cast<void *>(rightValue);
>+    }
>+};
>
> // Used for immediates which require relocation.
> struct ImmGCPtr
>@@ -123,6 +144,19 @@ struct ImmGCPtr
>         JS_ASSERT(!IsPoisonedPtr(ptr));
>     }
> };
>+// Used for immediates which require relocation for StringPointer.
>+struct ImmGCStringPtr
>+{
>+    uintptr_t leftValue;
>+    uintptr_t rightValue;
>+
>+    explicit ImmGCStringPtr(const gc::Cell *lptr,const gc::Cell *rptr)
>+     : leftValue(reinterpret_cast<uintptr_t>(lptr)), rightValue(reinterpret_cast<uintptr_t>(rptr))
>+    {
>+        JS_ASSERT(!IsPoisonedPtr(lptr));
>+        JS_ASSERT(!IsPoisonedPtr(rptr));
>+    }
>+};
>
> // Specifies a hardcoded, absolute address.
> struct AbsoluteAddress {
>diff --git a/js/src/ion/shared/Lowering-shared.cpp b/js/src/ion/shared/Lowering-shared.cpp
>index 577196d..6de028e 100644
>--- a/js/src/ion/shared/Lowering-shared.cpp
>+++ b/js/src/ion/shared/Lowering-shared.cpp
>@@ -36,6 +36,14 @@ LIRGeneratorShared::visitConstant(MConstant *ins)
> }
>
> bool
>+LIRGeneratorShared::visitStringConstant(MStringConstant *ins)
>+{
>+    const Value &lv = ins->leftValue();
>+    const Value &rv = ins->rightValue();
>+    return (define(new LStringPointer(rv.toString(),lv.toString()), ins));
>+}
>+
>+bool
> LIRGeneratorShared::defineTypedPhi(MPhi *phi, size_t lirIndex)
> {
>     LPhi *lir = current->getPhi(lirIndex);
>diff --git a/js/src/ion/shared/Lowering-shared.h b/js/src/ion/shared/Lowering-shared.h
>index d1439fc..2233ba7 100644
>--- a/js/src/ion/shared/Lowering-shared.h
>+++ b/js/src/ion/shared/Lowering-shared.h
>@@ -166,6 +166,7 @@ class LIRGeneratorShared : public MInstructionVisitorWithDefaults
>
>   public:
>     bool visitConstant(MConstant *ins);
>+    bool visitStringConstant(MStringConstant *ins);
>
>     // Whether to generate typed reads for element accesses with hole checks.
>     static bool allowTypedElementHoleCheck() {
>diff --git a/js/src/ion/shared/Lowering-x86-shared.cpp b/js/src/ion/shared/Lowering-x86-shared.cpp
>index 95496d7..c302311 100644
>--- a/js/src/ion/shared/Lowering-x86-shared.cpp
>+++ b/js/src/ion/shared/Lowering-x86-shared.cpp
>@@ -166,3 +166,12 @@ LIRGeneratorX86Shared::visitConstant(MConstant *ins)
>     return LIRGeneratorShared::visitConstant(ins);
> }
>
>+bool
>+LIRGeneratorX86Shared::visitStringConstant(MStringConstant *ins)
>+{
>+    // Emit non-double constants at their uses.
>+    if (ins->canEmitAtUses())
>+        return emitAtUses(ins);
>+
>+    return LIRGeneratorShared::visitStringConstant(ins);
>+}
>\ No newline at end of file
>diff --git a/js/src/ion/shared/Lowering-x86-shared.h b/js/src/ion/shared/Lowering-x86-shared.h
>index 797f552..a48310b 100644
>--- a/js/src/ion/shared/Lowering-x86-shared.h
>+++ b/js/src/ion/shared/Lowering-x86-shared.h
>@@ -28,6 +28,7 @@ class LIRGeneratorX86Shared : public LIRGeneratorShared
>     bool visitGuardShape(MGuardShape *ins);
>     bool visitPowHalf(MPowHalf *ins);
>     bool visitConstant(MConstant *ins);
>+    bool visitStringConstant(MStringConstant *ins);
>     bool visitAsmJSNeg(MAsmJSNeg *ins);
>     bool visitAsmJSUDiv(MAsmJSUDiv *ins);
>     bool visitAsmJSUMod(MAsmJSUMod *ins);

Comment 18

[Masaya Iseki[:isk](UTC+9)]

This patch is still work-in-progress.

I don't know what to do next.
I make visitStringPointer.
bool
CodeGenerator::visitStringPointer(LStringPointer *lir)
{
    if (lir->kind() == LStringPointer::GC_THING) {
        masm.movePtr(ImmGCPtr(lir->gclptr()), ToRegister(lir->output()));
    } else {
        masm.movePtr(ImmWord(lir->lptr(),lir->rptr()), ToRegister(lir->output()));
    }
    return true;
}

I'd like to move rptr and lptr together . ImmWord and ImmGCPtr constructer recieve a single ptr argument.
If I make new ImmGCStringPtr or ImmStringWord structure, I will have to add assembler(movq).

Is there any way to solve this problem?

Comment 19

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

(In reply to iseki.m.aa from comment #18)
> This patch is still work-in-progress.
> 
> I don't know what to do next.
> I make visitStringPointer.
> bool
> CodeGenerator::visitStringPointer(LStringPointer *lir)
> {
>     if (lir->kind() == LStringPointer::GC_THING) {
>         masm.movePtr(ImmGCPtr(lir->gclptr()), ToRegister(lir->output()));
>     } else {
>         masm.movePtr(ImmWord(lir->lptr(),lir->rptr()),
> ToRegister(lir->output()));
>     }
>     return true;
> }
> 
> I'd like to move rptr and lptr together . ImmWord and ImmGCPtr constructer
> recieve a single ptr argument.
> If I make new ImmGCStringPtr or ImmStringWord structure, I will have to add
> assembler(movq).
> 
> Is there any way to solve this problem?

the function isStringConstant in Value.h is unused and not needed as all JSStrings content are immutable from the JavaScript perspective, even if the underlying structure of the storage might change.

Strings are always GC things.  So there is no point of having a GC kind stored in the LStringPointer.  Another way to go would be to store a vector of MConstant, in the right order on the MStringConstant, such as the resulting constant correspond to the result of the concatenation of all constant strings contained in the vector.  There is no point at duplicating the value on the LStringPointer, as the define function should set the "mir_" member of LStringPointer to target.  Adding a function “MStringConstant *mir() const { return mir_; }” should do the trick and give you the ability to get the "Value" on each MConstant stored in the MStringConstant vector.

(In reply to iseki.m.aa from comment #18)
> This patch is still work-in-progress.
> 
> I don't know what to do next.
> I make visitStringPointer.
> bool
> CodeGenerator::visitStringPointer(LStringPointer *lir)
> {
>     if (lir->kind() == LStringPointer::GC_THING) {
>         masm.movePtr(ImmGCPtr(lir->gclptr()), ToRegister(lir->output()));
>     } else {
>         masm.movePtr(ImmWord(lir->lptr(),lir->rptr()),
> ToRegister(lir->output()));
>     }
>     return true;
> }
> 
> I'd like to move rptr and lptr together . ImmWord and ImmGCPtr constructer
> recieve a single ptr argument.
> If I make new ImmGCStringPtr or ImmStringWord structure, I will have to add
> assembler(movq).
> 
> Is there any way to solve this problem?

What you should add is a PatchableImmGCPtr(), and add it into a list stored as part of the CodeGeneratorShared, which will be used to do the string concatenation when the code is allocated in CodeGenerator::Link()

the PatchableImmGCPtr should store a way to construct the concatenated string as well as the offset which has to be patched with the newly allocated value.

PatchableImmGCPtr should inherit from ImmGCPtr, and initialize the ImmGCPtr parent with a dummy value which is different than 0, which can be identified as a non-correctly initialized value.

class MacroAssembler;

class PatchableImmGCPtr : public ImmGCPtr {
  private:
    size_t offset_;

  public:
    PatchableImmGCPtr()
      : ImmGCPtr(0x12345678),
        offset_(0)
    { }

    void bind(size_t offset) {
      offset_ = offset;
    }

    virtual patch(MacroAssembler &masm, JSContext *cx) const = 0;
};

Then you will need to specialize the PatchableImmGCPtr such as the “patch(MacroAssembler &masm, JSContext *cx)” builds the concatenated string out of the data originally stored on the MConstantString, and patch the code at the "offset_" location such as the generated code contain the Value to the newly allocated JSString* which contains the result of the concatenation.


Can you add more context into your patches (-U8) such as the review tools that are available on Bugzilla understand your patches and ease the way I can comment on them, Thanks.

You did some great improvement since the last time.  I am really happy to see you working again on this bug even if this is not an easy first bug.

Comment 20

[Masaya Iseki[:isk](UTC+9)]

>Another way to go would be to store a vector of MConstant, in the right order on the MStringConstant, such as the resulting >constant correspond to the result of the concatenation of all constant strings contained in the vector.  There is no point at >duplicating the value on the LStringPointer, as the define function should set the "mir_" member of LStringPointer to target.  >Adding a function “MStringConstant *mir() const { return mir_; }” should do the trick and give you the ability to get the >"Value" on each MConstant stored in the MStringConstant vector.

Sorry, I can't understand well.
Where do I make vector<MConstant>? in MConcat::foldsTo?
Where do I add "MStringConstant" ? in MConstant?

Comment 21

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

(In reply to iseki.m.aa from comment #20)
> >Another way to go would be to store a vector of MConstant, in the right order on the MStringConstant, such as the resulting >constant correspond to the result of the concatenation of all constant strings contained in the vector.  There is no point at >duplicating the value on the LStringPointer, as the define function should set the "mir_" member of LStringPointer to target.  >Adding a function “MStringConstant *mir() const { return mir_; }” should do the trick and give you the ability to get the >"Value" on each MConstant stored in the MStringConstant vector.
> 
> Sorry, I can't understand well.
> Where do I make vector<MConstant>? in MConcat::foldsTo?
> Where do I add "MStringConstant" ? in MConstant?

The idea would be that you add a MStringConstant in the MIR.h file, and store a vector of MConstant.  When MConstant & MStringConstant are folded together, you just collect them in the right order in the vector.

But I think it would be better to ask somebody who know the engine better before doing lots of modifications that we could handle properly by allocating objects during the MIR transformations.  We already have PJS which is doing GC allocations in parallel, and we should re-use what they are doing instead of making some complicated work-around, which would have to be done every time.

Comment 22

[Masaya Iseki[:isk](UTC+9)]

I understand your idea , thanks.

I think it is difficult to concat in IonMonkey(it means after IonCompile).
I suggest that I concat string constant in parser.
I found source code to fold constants in "mozilla-central/js/src/frontend/FoldConstants.cpp".
But this method can fold only if all term is a string or number.
If I can use this method when any term include things except a string and number, I can fix this bug.
But I don't know how to change this method for doing this.

Comment 23

[Masaya Iseki[:isk](UTC+9)]

Attachment: I change parser.

Comment 24

[Masaya Iseki[:isk](UTC+9)]

Sorry, I don't know how to add more context my patches.
I paste git diff. 

I change parser.
I folds constants before compile phase.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This page should help you generate diffs with the right amount of context.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 26

[Masaya Iseki[:isk](UTC+9)]

Comment on attachment 791838 [details]
I change parser.

# HG changeset patch
# User iseki <[e-mail]>

[PATCH] Bug 852791 - Change the processing of constant folding to fold constant If any term is not a string or number literal. r = Nicolas B. Pierron [:nbp]

---
 js/src/frontend/FoldConstants.cpp | 102 +++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 40 deletions(-)

diff --git a/js/src/frontend/FoldConstants.cpp b/js/src/frontend/FoldConstants.cpp
index b78d821..8d45288 100644
--- a/js/src/frontend/FoldConstants.cpp
+++ b/js/src/frontend/FoldConstants.cpp
@@ -532,55 +532,77 @@ FoldConstants<FullParseHandler>(JSContext *cx, ParseNode **pnp,
         /* FALL THROUGH */
       case PNK_ADD:
         if (pn->isArity(PN_LIST)) {
-            /*
-             * Any string literal term with all others number or string means
-             * this is a concatenation.  If any term is not a string or number
-             * literal, we can't fold.
-             */
+
             JS_ASSERT(pn->pn_count > 2);
-            if (pn->pn_xflags & PNX_CANTFOLD)
+
+            if (pn->pn_xflags & PNX_CANTFOLD && pn->pn_xflags != (PNX_CANTFOLD | PNX_STRCAT) )
                 return true;
-            if (pn->pn_xflags != PNX_STRCAT)
+            if (pn->pn_xflags != PNX_STRCAT && pn->pn_xflags != (PNX_CANTFOLD | PNX_STRCAT))
                 goto do_binary_op;
 
-            /* Ok, we're concatenating: convert non-string constant operands. */
-            size_t length = 0;
-            for (pn2 = pn1; pn2; pn2 = pn2->pn_next) {
-                if (!FoldType(cx, pn2, PNK_STRING))
-                    return false;
-                /* XXX fold only if all operands convert to string */
-                if (!pn2->isKind(PNK_STRING))
+            // we calculate the times of folding constants.
+            size_t cnt = 0;
+
+            ParseNode *prev = NULL;
+            bool isFold = false;
+            // if fold point is last , we must change pn_tail.
+            bool isLast = false;
+
+            for (pn2 = pn1; pn2;) {
+                isLast = false;
+                if (!prev) {
+                    pn2 = pn2->pn_next;
+                    prev = pn1;
+                    continue;
+                }
+                if (!pn2->isKind(PNK_STRING) || !prev->isKind(PNK_STRING)) {
+                    prev = pn2;
+                    pn2 = pn2->pn_next;
+                    continue;
+                }
+
+                isFold = true;
+                RootedString left(cx, prev->pn_atom);
+                RootedString right(cx, pn2->pn_atom);
+                RootedString str(cx, ConcatStrings<CanGC>(cx, left, right));
+                if (!str)
+                    continue;
+
+                prev->pn_atom = AtomizeString<CanGC>(cx, str);
+                if (!prev->pn_atom)
+                    continue;
+
+                prev->setKind(PNK_STRING);
+                prev->setOp(JSOP_STRING);
+                prev->setArity(PN_NULLARY);
+
+                prev->pn_next = pn2->pn_next;
+                parser->handler.freeTree(pn2);
+                pn2 = prev->pn_next;
+                ++cnt;
+                isLast = true;
+            }
+            if (!isFold) {
+                if (pn->pn_xflags & PNX_CANTFOLD)
                     return true;
-                length += pn2->pn_atom->length();
+                if (pn->pn_xflags != PNX_STRCAT)
+                    goto do_binary_op;
             }
 
-            /* Allocate a new buffer and string descriptor for the result. */
-            jschar *chars = cx->pod_malloc<jschar>(length + 1);
-            if (!chars)
-                return false;
-            chars[length] = 0;
-            JSString *str = js_NewString<CanGC>(cx, chars, length);
-            if (!str) {
-                js_free(chars);
-                return false;
+            // if we can fold one constant , arity is not PN_LIST.
+            if (pn->pn_count == cnt+1) {
+                pn->pn_atom = pn1->pn_atom;
+                if (!pn->pn_atom)
+                    return false;
+                pn->setKind(PNK_STRING);
+                pn->setOp(JSOP_STRING);
+                pn->setArity(PN_NULLARY);
+                break;
             }
-
-            /* Fill the buffer, advancing chars and recycling kids as we go. */
-            for (pn2 = pn1; pn2; pn2 = parser->handler.freeTree(pn2)) {
-                JSAtom *atom = pn2->pn_atom;
-                size_t length2 = atom->length();
-                js_strncpy(chars, atom->chars(), length2);
-                chars += length2;
+            if (isLast) {
+                pn->pn_tail = &(prev->pn_next);
             }
-            JS_ASSERT(*chars == 0);
-
-            /* Atomize the result string and mutate pn to refer to it. */
-            pn->pn_atom = AtomizeString<CanGC>(cx, str);
-            if (!pn->pn_atom)
-                return false;
-            pn->setKind(PNK_STRING);
-            pn->setOp(JSOP_STRING);
-            pn->setArity(PN_NULLARY);
+            pn->pn_count -= cnt;
             break;
         }
 
-- 
1.7.12.4 (Apple Git-37)

Comment 27

[Masaya Iseki[:isk](UTC+9)]

Attachment: I use https://github.com/jlebar/moz-git-tools.

Comment 28

[Masaya Iseki[:isk](UTC+9)]

Attachment: Patch to fix bug 852791

I use https://github.com/jlebar/moz-git-tools.

Comment 29

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

Comment on attachment 791851 [details] [diff] [review]
Patch to fix bug 852791

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

I am sorry, as mention in the bug title and in all previous comments, this modifications should be isolated to IonMonkey.

I do not think this is heading in any good direction.
I will remove my-self as a mentor of this bug as *it is not trivial*, even for me.  So I do not think this is a good first bug anymore until we manage to get a way to allocate anything during the parallel compilation.

I am sorry that you did not got a good first bug.  If you want, you can work on Bug 900285, which is made to prevent other from doing mistakes, and you already mentioned that you want to work on it.

Comment 30

[Masaya Iseki[:isk](UTC+9)]

This bug fixed in bug930565.