1171945 - [MBaselineCache] Create MIR/LIR

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Add platform in ionmonkey for sharedcaches

This contains all logic to have shared stubs in ionmonkey.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Add platform in ionmonkey for sharedcaches

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Move BinaryArith from Baseline to Shared stubs

Moves the required classes from BaselineIC.h to SharedIC.h.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part 3: Make changes to BinaryAirth to work with ionmonkey

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Part 4: Make it possible to use BaselineDebugMode in ionmonkey context

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8621575 [details] [diff] [review]
Part 1: Add platform in ionmonkey for sharedcaches

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

Looks good, just a few nits below.

::: js/src/jit/CodeGenerator.cpp
@@ +7886,5 @@
>      }
>  };
>  
>  bool
> +CodeGenerator::linkSharedStubs(JSContext *cx)

Nit: JSContext* cx

@@ +8131,5 @@
> +        entry = sharedStubs_[i].entry;
> +        entry.fixupReturnOffset(masm);
> +        Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, label),
> +                                           ImmPtr(&entry),
> +                                           ImmPtr((void*)-1));

This will probably crash with the --non-writable-jitcode option I added; easiest fix is to move this code inside the AutoWritableJitCode "if" in this function.

::: js/src/jit/Ion.cpp
@@ +942,5 @@
>  
>      uint32_t offsetCursor = sizeof(IonScript);
>  
> +    new (script->fallbackStubSpace()) FallbackICStubSpace();
> +    offsetCursor += paddedFallbackICStubSpace;

If all scripts have exactly one FallbackICStubSpace, we should just add it as a member to the IonScript class.

::: js/src/jit/MIR.h
@@ +7096,5 @@
> +class MSharedStub
> +{
> +    JSOp jsop_;
> +    JSScript *script_;
> +    jsbytecode *pc_;

We don't need these fields (and this class): all stubs will have a resume point, so you can do what CodeGeneratorShared::addCache and other places do:

JSScript* script = mir->block()->info().script();
jsbytecode* pc = mir->resumePoint()->pc();
JSOp jsop = JSOp(*pc);

And remove the script/pc arguments below.

::: js/src/jit/shared/Lowering-shared-inl.h
@@ +570,5 @@
> +
> +    ensureDefined(mir);
> +#if defined(JS_NUNBOX32)
> +    LUse type(op.typeReg(), useAtStart);
> +    type.setVirtualRegister(mir->virtualRegister());

Nit: maybe change this:

    LUse(Register reg, uint32_t virtualRegister) {
        set(FIXED, reg.code(), false);

to

    LUse(Register reg, uint32_t virtualRegister, bool useAtStart = false) {
        set(FIXED, reg.code(), useAtStart);

So you can do:

lir->setOperand(0, LUse(op.typeReg(), mir->virtualRegister(), useAtStart));

And same for the payload and x64.

@@ +577,5 @@
> +
> +    lir->setOperand(n, type);
> +    lir->setOperand(n + 1, payload);
> +#elif defined(JS_PUNBOX64)
> +    LUse value(op.typeReg(), useAtStart);

valueReg()

::: js/src/jit/shared/Lowering-shared.h
@@ +157,5 @@
>  
>      // Adds a use at operand |n| of a value-typed insturction.
>      inline void useBox(LInstruction* lir, size_t n, MDefinition* mir,
>                         LUse::Policy policy = LUse::REGISTER, bool useAtStart = false);
> +    inline void useFixedBox(LInstruction* lir, size_t n, MDefinition* mir,

Hm there's also useBoxFixed in Lowering-x86.h Use the same name here to be consistent?

(Maybe we should remove the platform-specific one later..)

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8621582 [details] [diff] [review]
Part 3: Make changes to BinaryAirth to work with ionmonkey

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

Nice!

::: js/src/jit/JitOptions.cpp
@@ +104,5 @@
>      // Toggle whether eager scalar replacement is globally disabled.
>      SET_DEFAULT(disableScalarReplacement, false);
>  
>      // Toggles whether shared stubs are used in Ionmonkey.
> +    SET_DEFAULT(disableSharedStubs, false);

Revert this change

::: js/src/jit/MIR.cpp
@@ +2585,5 @@
>  }
>  
> +bool
> +MBinaryArithInstruction::CanSpecialize(MDefinition* left, MDefinition* right,
> +                                       BaselineInspector* inspector, jsbytecode* pc)

Followup bug to move the infer() code into IonBuilder and make it work more like getprop?

::: js/src/jit/SharedIC.cpp
@@ +806,5 @@
> +        script = frame->script();
> +    } else {
> +        IonICEntry* entry = (IonICEntry*) stub_->icEntry();
> +        script = entry->script();
> +    }

This will probably appear in a lot of stubs so we could factor it out:

static JSScript*
SharedStubScript(BaselineFrame* frame)
{
    if (frame)
        return ...;
    return ...;
}

And maybe the same for the Engine var.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8621575 [details] [diff] [review]
Part 1: Add platform in ionmonkey for sharedcaches

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

::: js/src/jit/LIR-Common.h
@@ +3967,5 @@
> +  public:
> +    LIR_HEADER(UnarySharedStub)
> +
> +    const MBinarySharedStub* mir() const {
> +        return mir_->toBinarySharedStub();

s/MBinarySharedStub/MUnarySharedStub

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Add platform in ionmonkey for sharedcaches

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Part 5: Use tryXXXX instead of infer for jsop_binary

Instead of using a follow-up bug. This is the requested change from infer to using the tryXXX idea that jsop_getprop also uses.

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Part 5: Use tryXXXX instead of infer for jsop_binary

woeps, forgot a small detail.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8624203 [details] [diff] [review]
Part 1: Add platform in ionmonkey for sharedcaches

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

::: js/src/jit/Ion.cpp
@@ +1248,5 @@
> +        ICEntry& entry = sharedStubList()[i];
> +        if (!entry.hasStub())
> +            continue;
> +
> +        ICStub* lastStub = entry.firstStub();

I think we could share most of this code with BaselineScript. We could add something like this to SharedIC.h/cpp and call it in both cases:

void
PurgeOptimizedStubs(ICEntry& entry);

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8624294 [details] [diff] [review]
Part 5: Use tryXXXX instead of infer for jsop_binary

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

Looks good, some comments below.

::: js/src/jit/IonBuilder.cpp
@@ +7278,5 @@
> +        if (!ins->emptyResultTypeSet())
> +            continue;
> +
> +        LifoAlloc* alloc = GetJitContext()->temp->lifoAlloc();
> +        TemporaryTypeSet* types = alloc->new_<TemporaryTypeSet>();

You can use alloc_->lifoAlloc() here.

::: js/src/jit/MIR.cpp
@@ +2219,5 @@
> +                             MDefinition* left, MDefinition* right)
> +{
> +    switch (op) {
> +        case Op_Add:
> +            return MAdd::New(alloc, left, right);

Nit: 2 space indents for case labels and case statements.

@@ +2235,5 @@
> +}
> +
> +bool
> +MBinaryArithInstruction::possiblyDouble(TempAllocator& alloc, MDefinition::Opcode op,
> +                                        MDefinition *left, MDefinition *right)

As discussed, maybe we should make this a method and then IonBuilder can call it on the instruction and after that call setSpecialization/setResultType.

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: Part 5: Use tryXXXX instead of infer for jsop_binary

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8626196 [details] [diff] [review]
Part 5: Use tryXXXX instead of infer for jsop_binary

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

::: js/src/jit/IonBuilder.cpp
@@ +4595,5 @@
> +    // One of the inputs need to be a number.
> +    if (!IsNumberType(left->type()) && !IsNumberType(right->type()))
> +        return true;
> +
> +    MDefinition::Opcode def_op = JSOpToMDefinition(op);

Style nit: defOp

@@ +4597,5 @@
> +        return true;
> +
> +    MDefinition::Opcode def_op = JSOpToMDefinition(op);
> +    MBinaryArithInstruction* ins = MBinaryArithInstruction::New(alloc(), def_op, left, right);
> +    ins->setNumber(alloc(), inspector, pc);

Nit: setNumber is a bit unclear. setNumberSpecialization?

::: js/src/jit/MIR.cpp
@@ +2222,5 @@
> +                                   jsbytecode* pc)
> +{
> +    setSpecialization(MIRType_Double);
> +
> +    // Look if it possible to use Int32.

Nit: "it's", or maybe "// Try to specialize as int32."

@@ +2234,5 @@
> +    }
> +}
> +
> +bool
> +MBinaryArithInstruction::possiblyDouble(TempAllocator& alloc)

Nit: constantDoubleResult? Or maybe inline this function in its only caller.

Comment 16

[Pulsebot]

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

Comment 17

[Hannes Verschore [:h4writer]]

Attachment: Patch 1: Use tryXXX structure for jsop_binary in IonBuilder

Comment 18

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Add platform in ionmonkey for sharedcaches

Comment 19

[Hannes Verschore [:h4writer]]

Attachment: Part 3: Move BinaryArith from Baseline to Shared stubs

Comment 20

[Hannes Verschore [:h4writer]]

Attachment: Part 4: Make changes to BinaryAirth to work with ionmonkey

Comment 21

[Hannes Verschore [:h4writer]]

Attachment: Part 5: Make it possible to use BaselineDebugMode in ionmonkey context

Comment 22

[Hannes Verschore [:h4writer]]

Attachment: Part 6: Use binarystub in binary_arith

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dfb6e5f8223
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e954b36956
https://hg.mozilla.org/integration/mozilla-inbound/rev/89d8edcaee45
https://hg.mozilla.org/integration/mozilla-inbound/rev/110b6a5da6b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5377759a3145

Comment 25

[Nathan Froyd [:froydnj]]

Sorry, had to back this out for SM ARM compilation bustage that was hidden on your push by another push behind yours:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e1c580b5229b
https://treeherder.mozilla.org/logviewer.html#?job_id=13046193&repo=mozilla-inbound

It was so small, I hate to back it out, but it was too late in the day to find you. :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/04d727150d5d

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5484f536cd1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ecb6c04941
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2733714790
https://hg.mozilla.org/integration/mozilla-inbound/rev/95a3166bdb1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f08d8ca4ca83

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/5484f536cd1a
https://hg.mozilla.org/mozilla-central/rev/a8ecb6c04941
https://hg.mozilla.org/mozilla-central/rev/0f2733714790
https://hg.mozilla.org/mozilla-central/rev/95a3166bdb1c
https://hg.mozilla.org/mozilla-central/rev/f08d8ca4ca83

Comment 28

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Fix-build-failure-caused-by-Bug-117.patch

Comment 29

[Hannes Verschore [:h4writer]]

(In reply to Heiher from comment #28)
> Created attachment 8651588 [details] [diff] [review]
> 0001-IonMonkey-MIPS32-Fix-build-failure-caused-by-Bug-117.patch

Can you add this in bug 1197665 ? R+ btw ;)