[MBaselineCache] Create MIR/LIR

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Depends on 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(6 attachments, 10 obsolete attachments)

28.45 KB, patch
h4writer
: review+
h4writer
: checkin+
Details | Diff | Splinter Review
60.02 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
54.51 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
21.34 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
2.72 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
3.30 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
Assignee

Description

4 years ago
No description provided.
Assignee

Updated

4 years ago
Blocks: 1161516
Assignee

Comment 1

4 years ago
This contains all logic to have shared stubs in ionmonkey.
Assignee: nobody → hv1989
Attachment #8621570 - Flags: review?(jdemooij)
Assignee

Updated

4 years ago
Attachment #8621570 - Flags: review?(jdemooij)
Assignee

Comment 2

4 years ago
Attachment #8621575 - Flags: review?(jdemooij)
Assignee

Updated

4 years ago
Attachment #8621570 - Attachment is obsolete: true
Assignee

Comment 3

4 years ago
Moves the required classes from BaselineIC.h to SharedIC.h.
Attachment #8621581 - Flags: review?(jdemooij)
Assignee

Updated

4 years ago
Attachment #8621581 - Attachment description: Move BinaryArith from Baseline to Shared stubs → Part 2: Move BinaryArith from Baseline to Shared stubs
Assignee

Updated

4 years ago
Attachment #8621575 - Attachment description: Add platform in ionmonkey for sharedcaches → Part 1: Add platform in ionmonkey for sharedcaches
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..)
Attachment #8621575 - Flags: review?(jdemooij)
Attachment #8621581 - Flags: review?(jdemooij) → review+
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.
Attachment #8621582 - Flags: review?(jdemooij) → review+
Attachment #8621584 - Flags: review?(jdemooij) → review+
Assignee

Comment 8

4 years ago
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
Assignee

Comment 9

4 years ago
Attachment #8621575 - Attachment is obsolete: true
Attachment #8624203 - Flags: review?(jdemooij)
Assignee

Comment 10

4 years ago
Instead of using a follow-up bug. This is the requested change from infer to using the tryXXX idea that jsop_getprop also uses.
Attachment #8624248 - Flags: review?(jdemooij)
Assignee

Comment 11

4 years ago
woeps, forgot a small detail.
Attachment #8624248 - Attachment is obsolete: true
Attachment #8624248 - Flags: review?(jdemooij)
Attachment #8624294 - Flags: review?(jdemooij)
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);
Attachment #8624203 - Flags: review?(jdemooij) → review+
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.
Attachment #8624294 - Flags: review?(jdemooij)
Assignee

Comment 14

4 years ago
Attachment #8624294 - Attachment is obsolete: true
Attachment #8626196 - Flags: review?(jdemooij)
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.
Attachment #8626196 - Flags: review?(jdemooij) → review+
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Comment 17

4 years ago
Attachment #8621581 - Attachment is obsolete: true
Attachment #8621582 - Attachment is obsolete: true
Attachment #8621584 - Attachment is obsolete: true
Attachment #8624203 - Attachment is obsolete: true
Attachment #8626196 - Attachment is obsolete: true
Attachment #8647987 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8647987 - Flags: checkin+
Depends on: 1195588
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
Depends on: 1197604
Assignee

Comment 29

4 years ago
(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 ;)
Assignee

Updated

4 years ago
Depends on: 1197665

Updated

4 years ago
Attachment #8651588 - Attachment is obsolete: true
Attachment #8651588 - Flags: review?(hv1989)
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee

Updated

4 years ago
Duplicate of this bug: 1168757
Depends on: 1201469
Assignee

Updated

4 years ago
Depends on: 1207475
Depends on: 1246200
Depends on: 1250964
Assignee

Updated

3 years ago
No longer depends on: 1250964
Depends on: 1325344
You need to log in before you can comment on or make changes to this bug.