Closed Bug 670784 Opened 10 years ago Closed 10 years ago

IonMonkey: implement TableSwitch

Categories

(Core :: JavaScript Engine, defect)

All
Other
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(6 files, 10 obsolete files)

3.06 KB, patch
dvander
: review+
Details | Diff | Splinter Review
707 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
6.67 KB, patch
dvander
: review+
Details | Diff | Splinter Review
7.55 KB, patch
dvander
: review+
Details | Diff | Splinter Review
41.68 KB, patch
dvander
: review+
Details | Diff | Splinter Review
2.15 KB, patch
dvander
: review+
Details | Diff | Splinter Review
Attached patch review2.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

I implement TableSwitch in IonMonkey. It isn't done yet, but most functionality is in and working. I just want to get some feedback if I did it correctly and I have some questions aswell.

1) There is a TODO in my patch about adding a srcnote to the break (GOTO) statement. Would that be a sensible thing to do? Because now I'm just taking all GOTO's that don't belong to some other control flow instruction. 

2) I've adjusted MControlInstruction to use jsvector to list successors. But I think we agreed not to do that? Actually there should be a split from MControlInctruction to a class that has exactly X successors. E.g. something that MGoto (1 successors), MTest (2 successors) would implement. So those are scripted statically. And another class (probably only MTableSwitch) that uses jsvector to dynamically create the list of successors. (Every MTableSwitch can have arbitrary amount of successors/cases). I actually just don't know how I should call that first class, that has always X successors.

3) I adjusted LoopInfo to ControlFlowInfo. Because I needed something alike for breaks in a tableswitch. Both contain the same information, so I just altered it.

4) LTableSwitch isn't implemented yet. I don't know what I should put in there actually. Should it make the jump table again? Because I thought the LIR would more or less resemble the eventually ASM output a bit. But are the pc's already know? ... So this one is a bit vague for me.
Status: UNCONFIRMED → NEW
Ever confirmed: true
5) I was also wondering if it makes more sense to create all case blocks directly and just keeping them in a jsvector* in the tableswitch CFGState. Atm I'm just adding them when I encounter them. So this means I have to get the MTableSwitch instruction and add the block as successor there. I think the former would be nicer?
> 1) There is a TODO in my patch about adding a srcnote to the break (GOTO)
> statement. Would that be a sensible thing to do? Because now I'm just taking
> all GOTO's that don't belong to some other control flow instruction. 

I prefer the srcnote unless it's clear from the emitter that the GOTO could not be anything else. We want to be sure there is no possibility for ambiguity. If you're sure of that, we need some way to assert on it in case someone else changes the emitter in the future.

> 2) I've adjusted MControlInstruction to use jsvector to list successors. But
> I think we agreed not to do that? Actually there should be a split from
> MControlInctruction to a class that has exactly X successors. E.g. something
> that MGoto (1 successors), MTest (2 successors) would implement. So those
> are scripted statically. And another class (probably only MTableSwitch) that
> uses jsvector to dynamically create the list of successors.

MControlInstruction just needs functions like:
  virtual size_t numSuccessors() const = 0;
  virtual MBasicBlock *getSuccessor() const = 0;
  virtual void replaceSuccessor(size_t i, MBasicBlock *successor) = 0;

MTest, MGoto would have static buffers local to their classes, MTableSwitch would have its js::Vector, and all three would implement those virtual functions according to their class.

It's probably not worth abstracting further until we get more control instructions.

> 3) I adjusted LoopInfo to ControlFlowInfo. Because I needed something alike
> for breaks in a tableswitch. Both contain the same information, so I just
> altered it.

Sounds good.

> 4) LTableSwitch isn't implemented yet. I don't know what I should put in
> there actually. Should it make the jump table again? Because I thought the
> LIR would more or less resemble the eventually ASM output a bit. But are the
> pc's already know? ... So this one is a bit vague for me.

Yeah, a jump table is what you want. Code generation is pretty new, I'll come by and we can talk about it.

(In reply to comment #1)
> 5) I was also wondering if it makes more sense to create all case blocks
> directly and just keeping them in a jsvector* in the tableswitch CFGState.
> Atm I'm just adding them when I encounter them. So this means I have to get
> the MTableSwitch instruction and add the block as successor there. I think
> the former would be nicer?

The successor list has to be in the switch block anyway so it's probably simpler to just keep the switch instruction in the CFGState. It might be nicer to create it all at once rather than incrementally though. Just keep in mind that the |pc| field has to be accurate.
Ty for the answers. I'm busy with adjusting the patch and whenever I'm ready I will repost it. 
But for now I implemented the source note on the break (JSOP_GOTO) in a switch. So I probably wants this in mozilla-central first.
I saw in jsscan.h all SRC_* are taken. So I created SRC_SWITCHBREAK having the same number as SRC_SWITCH. Happens a lot looking at the file. So is that alright?
Attachment #545541 - Flags: review?(dvander)
Attached patch Patch (implementation 1) (obsolete) — Splinter Review
remarks 2-5 are implemented.

When doing remark one, I just did an overhaul. They both do the same, but I'm not sure which one is the best. The later one is more readable and has less code... But it could be that the first on is a bit faster (less condition checks).
Attachment #545286 - Attachment is obsolete: true
Attached patch Patch (implementation 2) (obsolete) — Splinter Review
Comment on attachment 545541 [details] [diff] [review]
Add src note to break statement

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

::: js/src/shell/js.cpp
@@ +2212,5 @@
>              Sprint(sp, " function %u (%s)", index, !!bytes ? bytes.ptr() : "N/A");
>              break;
>            }
>            case SRC_SWITCH:
> +            if(js_GetOpcode(cx, script, script->code + offset) == JSOP_GOTO)

nit: space in between if and (
Attachment #545541 - Flags: review?(dvander) → review+
This landed on m-i, but was caught up in a mass backout.  I will reland this later if it appears it was not responsible for bustage.
http://hg.mozilla.org/mozilla-central/rev/d5ae5580508f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Attached patch patch 0 (obsolete) — Splinter Review
(Reopened because only part of patch has landed, the src note stuff)

So this takes all remarks into account. The lowering and code generation is also done. Except for the jump table. That's because some things aren't in place yet to do this (said dvander). If you want me to do it, I'll need some more information.

(I noticed some weird problems with TypeAnalyzer. So I think there is still a bug in it. I'll investigate on Monday and post a bug)
Assignee: general → hv1989
Attachment #545573 - Attachment is obsolete: true
Attachment #545575 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Dont worry about Typeanalyzer, it's bogus. It's removed and rewritten in my queue -- will post patch tomorrow
Attached patch patch wip2 (obsolete) — Splinter Review
Patch is complete except for jmp table.
Attachment #546398 - Attachment is obsolete: true
Attached patch tableswitch wip3 (obsolete) — Splinter Review
Working TableSwitch !!

Notes:
- what should I use as the default inline space for vectors?
- JM don't use a jumptable if number of cases > 256. I suppose we want this in IM too? Can I just put NYI? I suppose we want to use a LookUpSwitch then?
- I added the next function in Assembler-x86-shared.h 
 +    void jmp(const Register &base, const Register &index,/* Scale scale,*/ int32_t offset = 0){
As you see I didn't implement Scale yet and also I think this is to specific? What do we want to do with this?
Attachment #547743 - Attachment is obsolete: true
(In reply to comment #12)
> - what should I use as the default inline space for vectors?

Pick 0 if in doubt. 

> - JM don't use a jumptable if number of cases > 256. I suppose we want this
> in IM too?

Can we make it unlimited? Is there any reason to fail after 256?

> - I added the next function in Assembler-x86-shared.h 
>  +    void jmp(const Register &base, const Register &index,/* Scale scale,*/
> int32_t offset = 0){
> As you see I didn't implement Scale yet and also I think this is to
> specific? What do we want to do with this?

Want to add scale to Operand()? We will need it eventually:
  (1) Add a SCALE type to Operand::Kind
  (2) Add a 2-bit scale field after base_
  (3) Add another 5-bit reg field for the index.
  (4) Add necessary constructor(s) and accessors.
Attachment #548953 - Attachment is obsolete: true
Attachment #549521 - Flags: review?(dvander)
Attachment #549522 - Flags: review?(dvander)
Now again, but this time x64 works too
Attachment #549521 - Attachment is obsolete: true
Attachment #549536 - Flags: review?(dvander)
Attachment #549521 - Flags: review?(dvander)
Attachment #549522 - Attachment is obsolete: true
Attachment #549537 - Flags: review?(dvander)
Attachment #549522 - Flags: review?(dvander)
small nit in part2:

+    JS_ASSERT((void **)code->raw()+masm.size()+masm.dataSize() > jumpData);

should be 

+    JS_ASSERT((void **)code->raw()+masm.size()+masm.dataSize() >= jumpData);
Attached patch Implement uint16Splinter Review
Just tested number cases > 300 still works. Original bug report (591972) just mentions: "* A table switch is a switch with "constant integer cases"; this patch optimizes this to a jump table (when highest value - lowest value <= 256 to prevent excessive memory use)".

I had to add JSOP_UINT16, which is a no-brainer. If needed I can create a new bug for this?
Attachment #549545 - Flags: review?(dvander)
Comment on attachment 549536 [details] [diff] [review]
Part 1: adding some needed structures

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

r=me with changes (some comments are duplicated before I realized they were in both assembler files)

::: js/src/ion/shared/Assembler-shared.h
@@ +95,5 @@
> +enum Scale {
> +    TimesOne,
> +    TimesTwo,
> +    TimesFour,
> +    TimesEight

Move Scale near the definitions of Operand, since it's arch-specific.

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +113,5 @@
>          return masm.size();
>      }
> +    size_t dataSize() const {
> +        return dataSize_;
> +    }

More on this in a bit in the future patches, but I think we should find a way to use bytesNeeded() instead.

@@ +209,5 @@
>              JmpSrc prev = JmpSrc(label->use(j.offset()));
>              masm.setNextJump(j, prev);
>          }
>      }
> +    void jmp(const Operand &op, int32_t offset = 0){

Remove the |offset| field from here, and

@@ +212,5 @@
>      }
> +    void jmp(const Operand &op, int32_t offset = 0){
> +        switch (op.kind()) {
> +          case Operand::SCALE:
> +            masm.jmp_m(offset, op.base(), op.index(), op.scale());

Use op.disp() instead.

::: js/src/ion/x64/Assembler-x64.h
@@ +109,5 @@
>      explicit Operand(const FloatRegister &reg)
>        : kind_(FPREG),
>          base_(reg.code())
>      { }
> +    explicit Operand(const Register &base, const Register &index, Scale scale)

Add optional |int disp = 0| here

@@ +145,5 @@
>          JS_ASSERT(kind() == FPREG);
>          return (FloatRegisters::Code)base_;
>      }
>      int32 disp() const {
>          JS_ASSERT(kind() == REG_DISP);

|| kind() == SCALE

::: js/src/ion/x86/Assembler-x86.h
@@ +90,5 @@
>      explicit Operand(const FloatRegister &reg)
>        : kind_(FPREG),
>          base_(reg.code())
>      { }
> +    explicit Operand(const Register &base, const Register &index, Scale scale)

Add an optional |int disp = 0| here

@@ +126,5 @@
>          JS_ASSERT(kind() == FPREG);
>          return (FloatRegisters::Code)base_;
>      }
>      int32 disp() const {
>          JS_ASSERT(kind() == REG_DISP);

This should work for kind() == SCALE
Attachment #549536 - Flags: review?(dvander) → review+
Comment on attachment 549537 [details] [diff] [review]
Part 2: the actual tableswitch implementation

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

::: js/src/ion/IonBuilder.cpp
@@ +851,5 @@
>      return ControlStatus_Joined;
>  }
>  
>  IonBuilder::ControlStatus
> +IonBuilder::processTableSwitchCase(CFGState &state)

This function is really big. Could we split it up, perhaps into processNextTableSwitchCase() and processTableSwitchEnd()?

@@ +864,5 @@
> +
> +        // Add current block as predecessor if available.
> +        // This means the previous case didn't have a break statement.
> +        // So flow will continue in this block.
> +        if (current){

Space in between ) and {

@@ +910,5 @@
> +            }
> +
> +            // If there is current, this means the previous case flows into this one. (no break statement)
> +            // So current is a predecessor to successor 
> +            if (current){

Space in between ) and {

@@ +914,5 @@
> +            if (current){
> +                current->end(MGoto::New(successor));
> +                successor->addPredecessor(current);
> +            }
> +

Excess newline

@@ +915,5 @@
> +                current->end(MGoto::New(successor));
> +                successor->addPredecessor(current);
> +            }
> +
> +        } else if (current){

Space in between ) and {

@@ +930,5 @@
> +            // No break statements and no current
> +            // This means that control flow is cut-off from this point
> +            // (e.g. return statements).
> +            return ControlStatus_Ended;
> +        }

The control flow handling here can be simplified, to look like finalizeLoop() except with some slight modifications. I bet a lot of the code (at the very least, deferred edges) can be factored out. Basically you want something like:

> if (!state.breaks && !current)
>     return Ended
> if (state.breaks)
>     successor = createBreakCatchBlock(state.breaks)
> else
>     successor = newBlock
> if (current)
>     create edge from current -> successor

@@ +1005,5 @@
> +    CFGState *found = NULL;
> +    jsbytecode *target = pc + GetJumpOffset(pc);
> +    for (size_t i = switches_.length() - 1; i < switches_.length(); i--) {
> +        if (switches_[i].continuepc == target) {
> +            found = &cfgStack_[switches_[i].cfgEntry];

The parser and source note guarantee us that the break won't be part of a loop, right?

@@ +1289,5 @@
> +        // Test if the default case appears before this case.
> +        // If it does add the default case
> +        if(defaultpc < casepc) {
> +            tableswitch->addDefault(defaultcase);
> +        }

I don't quite understand this here - should this be defaultpc == casepc?

@@ +1294,5 @@
> +
> +        MBasicBlock *caseblock = newBlock(current, casepc);
> +        tableswitch->addCase(caseblock);
> +        if (!caseblock)
> +            return ControlStatus_Error;

Move the NULL check before the addCase()

@@ +1329,5 @@
> +    if (tableswitch->numSuccessors() == 1){
> +        state.stopAt = state.tableswitch.exitpc;
> +    } else {
> +        state.stopAt = tableswitch->getSuccessor(1)->pc();
> +    }

Why is successor 0 not the next successor?

::: js/src/ion/IonBuilder.h
@@ +146,5 @@
> +
> +                // MIR instruction
> +                MTableSwitch *ins;
> +
> +                // The number of current successor that get's mapped into a block. 

// The number of current successors that get mapped into a block.

::: js/src/ion/IonLowering.cpp
@@ +146,5 @@
> +
> +    // If there are no cases, the default case is always taken. 
> +    if (tableswitch->numSuccessors() == 1) {
> +        return add(new LGoto(tableswitch->getDefault()));        
> +    }

House style is to remove braces for one-liners.

@@ +165,5 @@
> +                if (switchval < tableswitch->low() || switchval > tableswitch->high())
> +                    return add(new LGoto(tableswitch->getDefault()));
> +
> +                return add(new LGoto(tableswitch->getCase(switchval-tableswitch->low())));
> +            }

Reading jsinterp.cpp for TABLESWITCH:

    int32_t i;
    if (rref.isInt32()) {
        i = rref.toInt32();
    } else {
        double d;
        /* Don't use JSDOUBLE_IS_INT32; treat -0 (double) as 0. */
        if (!rref.isDouble() || (d = rref.toDouble()) != (i = int32_t(rref.toDouble())))
            DO_NEXT_OP(len);
    }

It looks like booleans and objects should go to the default case as well.

::: js/src/ion/LIR-Common.h
@@ +196,5 @@
> +{
> +    MTableSwitch *mir_;
> +
> +    // Contains the position to code where pointer to the jumpTable should get patched 
> +    DataLabelPtr patchPos_;

I think I suggested this name but I don't like it anymore :) How about "DataPtrLabel" instead?

@@ +206,5 @@
> +                 const LDefinition &jumpTablePointer, MTableSwitch *mir)
> +      : mir_(mir)
> +#if(DEBUG) 
> +        , patchPos_()
> +#endif

Remove #ifdef DEBUG

@@ +221,5 @@
> +    void setPatchPos(DataLabelPtr patchPos) {
> +        patchPos_ = patchPos;
> +    }
> +
> +    DataLabelPtr patchPos() {

This function can be const

::: js/src/ion/MIR.h
@@ +551,5 @@
> +class MTableSwitch
> +  : public MControlInstruction,
> +    public TableSwitchPolicy
> +{
> +    Vector<MBasicBlock*, 0, IonAllocPolicy> successors;

successors_

@@ +555,5 @@
> +    Vector<MBasicBlock*, 0, IonAllocPolicy> successors;
> +    MDefinition *operand_;
> +    int32 low_;
> +    int32 high_;
> +    uint32 defaultcase;

defaultCase

@@ +563,5 @@
> +        low_(low),
> +        high_(high)
> +#if DEBUG
> +        , defaultcase(-1)
> +#endif

I would remove the #ifdef DEBUG here, initializing it is cheap.

@@ +609,5 @@
> +
> +    MBasicBlock *getCase(uint32 i) const {
> +        JS_ASSERT(i < successors.length());
> +
> +        if (i<defaultcase)

House style is to give operators some breathing room (i < defaultCase), here

@@ +612,5 @@
> +
> +        if (i<defaultcase)
> +            return getSuccessor(i);
> +
> +        return getSuccessor(i+1);

here

@@ +617,5 @@
> +    }
> +
> +    size_t numCases() const {
> +        return high()-low()+1;
> +    }

here

@@ +626,5 @@
> +        successors.append(block);
> +    }
> +
> +    void addCase(MBasicBlock *block) {
> +        JS_ASSERT(successors.length() < (int32)(high_-low_+1));

And here

@@ +632,5 @@
> +    }
> +
> +    MIRType requiredInputType(size_t index) const {
> +        return MIRType_Int32;
> +    }

This isn't needed anymore.

@@ +637,5 @@
> +
> +    MDefinition *getOperand(size_t index) const {
> +        JS_ASSERT(index == 0);
> +        return operand_;
> +    }

These functions might be shorter to implement (no manual asserting) if you use FixedArityList<MDefinition *, 1> instead of MDefinition * operand.

@@ +705,5 @@
>  class MTest
>    : public MAryControlInstruction<1>,
>      public BoxInputsPolicy
>  {
> +    MBasicBlock *successors[2];

A FixedArityList would make sense here.

@@ +766,5 @@
> +    MBasicBlock *getSuccessor(size_t i) const {
> +        JS_NOT_REACHED("There are no successors");
> +
> +        // I know there are special pointers to indicate freed memory.
> +        // Is there also one for this? Or is NULL just fine?

Add a FixedArityList<MBasicBlock *, 0> member :)

::: js/src/ion/TypePolicy.cpp
@@ +164,5 @@
>  
>  bool
>  BitwisePolicy::useSpecializedInput(MInstruction *ins, size_t index, MInstruction *special)
>  {
> +

Extra newline?

@@ +200,5 @@
> +        default: {
> +            // Unbox to Int32 in case of
> +            // MIRType_Object
> +            // MIRType_Value
> +            MInstruction *replace = MUnbox::New(in, MIRType_Int32);

The input of an Unbox must be a value, it can't take a typed node. But I think for an object you can take the default branch?

For doubles, you will need to do something - looking at jsinterp it looks like MToInt32 will work: MToInt32(double) would just deoptimize if the double couldn't fit in an int.

::: js/src/ion/x64/Assembler-x64.h
@@ +84,5 @@
>  static const Register ArgReg3 = rdx;
>  static const Register ArgReg4 = rcx;
>  #endif
>  
> +static const Scale ScalePtr = TimesEight;

ScalePointer, we can spare the vowels here :)

::: js/src/ion/x86/Assembler-x86.h
@@ +65,5 @@
>  static const Register JSReturnReg_Type = ecx;
>  static const Register JSReturnReg_Data = edx;
>  static const Register StackPointer = esp;
>  
> +static const Scale ScalePtr = TimesFour;

ScalePointer
Attachment #549537 - Flags: review?(dvander)
Comment on attachment 549545 [details] [diff] [review]
Implement uint16

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

::: js/src/ion/IonBuilder.cpp
@@ +416,5 @@
>        case JSOP_INT8:
>          return pushConstant(Int32Value(GET_INT8(pc)));
>  
> +      case JSOP_UINT16:
> +        return pushConstant(Int32Value((int32_t) GET_UINT16(pc)));

Shouldn't need the cast here, r=me with that
Attachment #549545 - Flags: review?(dvander) → review+
Depends on: 675395
Attachment #550933 - Flags: review?(dvander)
Removed some code and added some code, reflecting the changes of IonMonkey.
Attachment #549536 - Attachment is obsolete: true
Attachment #550940 - Flags: review?(dvander)
Attachment #550933 - Flags: review?(dvander) → review+
Comment on attachment 550940 [details] [diff] [review]
Part 1: adding some needed structures (again)

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

r=me with nits

::: js/src/ion/shared/CodeGenerator-x86-shared.h
@@ +75,5 @@
>      }
>      inline Operand ToOperand(const LDefinition *def) {
>          return ToOperand(def->output());
>      }
> +    inline Operand ToOperand(const Register &base, const Register &index, Scale scale, int32 disp = 0) {

Is this still needed, since you can just use the constructor?

::: js/src/ion/x64/Assembler-x64.h
@@ +89,5 @@
> +    TimesOne,
> +    TimesTwo,
> +    TimesFour,
> +    TimesEight,
> +    TimesSixteen

There is no *16 :(

::: js/src/ion/x86/Assembler-x86.h
@@ +70,5 @@
> +    TimesOne,
> +    TimesTwo,
> +    TimesFour,
> +    TimesEight,
> +    TimesSixteen

Same.
Attachment #550940 - Flags: review?(dvander) → review+
> This function is really big. Could we split it up, perhaps into processNextTableSwitchCase() and processTableSwitchEnd()?
>...
>The control flow handling here can be simplified, to look like finalizeLoop() except with some slight modifications. I bet a lot of the code (at the very least, deferred edges) can be factored out. Basically you want something like:
>...
Sure, done.

> @@ +1289,5 @@
> > +        // Test if the default case appears before this case.
> > +        // If it does add the default case
> > +        if(defaultpc < casepc) {
> > +            tableswitch->addDefault(defaultcase);
> > +        }
> I don't quite understand this here - should this be defaultpc == casepc?
=> Should be "defaultpc >= prevcasepc && defaultpc < casepc"

> I think I suggested this name but I don't like it anymore :) How about "DataPtrLabel" instead?
=> obsolete, so removed this. 

> @@ +1329,5 @@
> > +    if (tableswitch->numSuccessors() == 1){
> > +        state.stopAt = state.tableswitch.exitpc;
> > +    } else {
> > +        state.stopAt = tableswitch->getSuccessor(1)->pc();
> > +    }
> Why is successor 0 not the next successor?
=> successor 0 is the first successor. So the end of the first successor (0) is the start of the second successor (1).
=> That's why I use getSuccessor(1)->pc()

> The parser and source note guarantee us that the break won't be part of a loop, right?
=> Right

> It looks like booleans and objects should go to the default case as well.
> ...
> The input of an Unbox must be a value, it can't take a typed node. But I think for an object you can take the default branch?
> ...
> For doubles, you will need to do something - looking at jsinterp it looks like MToInt32 will work: MToInt32(double) would just deoptimize if the double couldn't fit in an int.
> ...
=> Updated the logic of TypePolicy and IonLowering to take this into account.
Attachment #549537 - Attachment is obsolete: true
Attachment #550970 - Flags: review?(dvander)
(In reply to David Anderson [:dvander] from comment #26)
> Comment on attachment 550940 [details] [diff] [review] [diff] [details] [review]
> Part 1: adding some needed structures (again)
> 
> Review of attachment 550940 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits
> 
> ::: js/src/ion/shared/CodeGenerator-x86-shared.h
> @@ +75,5 @@
> >      }
> >      inline Operand ToOperand(const LDefinition *def) {
> >          return ToOperand(def->output());
> >      }
> > +    inline Operand ToOperand(const Register &base, const Register &index, Scale scale, int32 disp = 0) {
> 
> Is this still needed, since you can just use the constructor?

No it isn't. Removed it.

> ::: js/src/ion/x64/Assembler-x64.h
> ::: js/src/ion/x86/Assembler-x86.h
> @@ +89,5 @@
> > +    TimesOne,
> > +    TimesTwo,
> > +    TimesFour,
> > +    TimesEight,
> > +    TimesSixteen
> 
> There is no *16 :(

Yeah, a brain twist made me think I needed it. But indeed isn't needed. So removed that too.

http://hg.mozilla.org/projects/ionmonkey/rev/48aebed719c2
http://hg.mozilla.org/projects/ionmonkey/rev/e32e1805a61f
Comment on attachment 550970 [details] [diff] [review]
Part 2: the actual tableswitch implementation (again)

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

Looks great! Just a few nits.

::: js/src/ion/IonBuilder.cpp
@@ +809,5 @@
>      return true;
>  }
>  
> +MBasicBlock *
> +IonBuilder::createBreakCatchBlock(DeferredEdge *edge, jsbytecode *pc)

Nice

@@ +1278,5 @@
> +        JS_ASSERT(casepc > pc && casepc <= exitpc);
> +
> +        // Test if the default case appears before this case.
> +        // If it does add the default case
> +        if(defaultpc >= prevcasepc && defaultpc < casepc)

Space in between "if" and (

@@ +1319,5 @@
> +    if (tableswitch->numSuccessors() == 1){
> +        state.stopAt = state.tableswitch.exitpc;
> +    } else {
> +        state.stopAt = tableswitch->getSuccessor(1)->pc();
> +    }

House style is remove braces if each line is a one-liner

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +255,5 @@
> + 
> +    // Create a label pointing to the jumptable
> +    // This gets patched after linking
> +    CodeLabel *label = new CodeLabel();
> +    masm.addCodeLabel(label);

I think this is fallible, so it needs an error check.

@@ +274,5 @@
> +    masm.bind(label->src());
> +
> +    for (uint j=0; j<ins->mir()->numCases(); j++) { 
> +        LBlock *caseblock = ins->mir()->getCase(j)->lir();
> +        LLabel *caseheader = caseblock->begin()->toLabel();

Can you use caseblock->label() here instead?
Attachment #550970 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/6bce7f59bb15

I'm keeping this alive for a few more days, because dvander told me to "remove the useSpecializedInput callback and implement the new callback for calling useAsType". Didn't know this before pushing, so will do this when I come back.
trivial change
Attachment #551823 - Flags: review?(dvander)
Attachment #551823 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/18313e89ae36

follow up: bug 677636 for measuring speed difference between the 3 ways to create the tableswitch
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 545541 [details] [diff] [review]
Add src note to break statement

I'm fixing (as part of the patch queue for bug 568142) the SRC_SWITCHBREAK type code to be 8 (to overlay SRC_ASSIGNOP), instead of 18. See the jsemit.h comment before the JSSrcNoteType enum about why it's best to match src note "shape". This patch wastes two useless offsets by reusing SRC_SWITCH's type-code.

/be
You need to log in before you can comment on or make changes to this bug.