Closed Bug 673870 Opened 13 years ago Closed 13 years ago

IonMonkey: Disentangle MoveResolver from CodeGenerator

Categories

(Core :: JavaScript Engine, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
JM's assembler had a nifty API for abstracting calls out to C, but it was fundamentally broken because it couldn't really handle difficult moves. In IonMonkey, we have move resolution! We can get to a call API like:

  void startCall(3 /* args *);
  void setArg(0, reg1);
  void setArg(1, Operand(reg2, 12));
  void setArg(2, reg3);
  void emitCall(fun);

The MacroAssembler could have a MoveGroupResolver, and just add the argument moves to it as the user supplies them. Once the user calls emitCall(), it could resolve everything then and there.

To make this work, this patch refactors the move resolver classes.

MoveGroupResolver no longer operates on LAllocation pairs, but a new pair of "MoveOperands". MoveOperand is some sort of compromise between LAllocation and Operand - unfortunately there is significant duplication. Users of MoveGroupResolver are responsible for giving it a list of MoveOperands. Its singleton now resides in MacroAssembler.

MoveResolverX86 really now could be combined entirely into MacroAssembler, but I left it separate for now. It's now significantly cleaner. One new bit of complexity is that it has to modify esp-based moves within cycles. It no longer needs a CodeGenerator and it instantiates as a temporary variable.
Attachment #548123 - Flags: review?(adrake)
Err, I'm not sure if we can ever generate a constant move in our register allocators. Virtual registers have no concept of "constant", only LAllocations, which wouldn't specify a register anyway and shouldn't be spilled.

So consider stuff relating to constants ("unordered moves") just gone.
Comment on attachment 548123 [details] [diff] [review]
patch

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

Just some minor stuff:

::: js/src/ion/MoveGroupResolver.cpp
@@ +53,5 @@
>  {
> +    PendingMove *pm = movePool_.allocate();
> +    if (!pm)
> +        return false;
> +    new (pm) PendingMove(from, to, kind);

can movePool_.allocate take the constructor args so we aren't doing placement new out here?

@@ +54,5 @@
> +    PendingMove *pm = movePool_.allocate();
> +    if (!pm)
> +        return false;
> +    new (pm) PendingMove(from, to, kind);
> +    pending_.insert(pm);

return pending_.insert(pm); ?

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

Nit: Trailing whitespace

::: js/src/ion/MoveGroupResolver.h
@@ +53,5 @@
> +    // This is similar to Operand, but carries more information. We're also not
> +    // guaranteed that Operand looks like this on all ISAs.
> +    class MoveOperand
> +    {
> +        enum Kind {

Nit: Implicit protected.

@@ +61,5 @@
> +        };
> +
> +        Kind kind_ : 2;
> +        uint32 code_ : 5;
> +        int32 disp_;

This seems a bit premature at the moment, we should have a follow-up bug for shaving bits once we know which ones to shave.

@@ +76,5 @@
> +            code_(reg.code()),
> +            disp_(disp)
> +        { }
> +        MoveOperand(const MoveOperand &other)
> +          : kind_(other.kind_), code_(other.code_), disp_(other.disp_)

Nit: Newlines

@@ +88,5 @@
> +        bool isDouble() const {
> +            return kind_ == FLOAT_REG;
> +        }
> +        bool isMemory() const {
> +            return kind_ >= ADDRESS;

What benefit is gained from >= vs == here?

@@ +122,5 @@
>      {
> +      protected:
> +        MoveOperand from_;
> +        MoveOperand to_;
> +        bool cycle_ : 1;

Same comment about bit fields like this.

@@ +131,5 @@
> +            DOUBLE
> +        };
> +
> +      protected:
> +        Kind kind_ : 1;

And here.

@@ +137,5 @@
>        public:
>          Move()
>          { }
> +        Move(const Move &other)
> +          : from_(other.from_), to_(other.to_), cycle_(other.cycle_), kind_(other.kind_)

Nit: Newlines

@@ +140,5 @@
> +        Move(const Move &other)
> +          : from_(other.from_), to_(other.to_), cycle_(other.cycle_), kind_(other.kind_)
> +        { }
> +        Move(const MoveOperand &from, const MoveOperand &to, Kind kind, bool cycle = false)
> +          : from_(from), to_(to), cycle_(cycle), kind_(kind)

And here.

@@ +186,1 @@
>      js::Vector<Move, 16, SystemAllocPolicy> orderedMoves_;

Related to the point above on bit shaving, we should also check for good values these inline sizes at some point.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +186,5 @@
> +                 ? SlotToStackOffset(a->toStackSlot()->slot())
> +                 : ArgToStackOffset(a->toArgument()->index());
> +    if (a->isStackSlot())
> +        return MoveOperand(StackPointer, disp);
> +    return MoveOperand(StackPointer, disp);

What's the difference between these two cases?

@@ +201,5 @@
> +        // No bogus moves.
> +        JS_ASSERT(*move.from() != *move.to());
> +
> +        const LAllocation *from = move.from();
> +        const LAllocation *to = move.to();

Microscopic Nit: Can the assert use from and to? I think it'd just look a little nicer.

@@ +215,5 @@
> +        MoveGroupResolver::Move::Kind kind = from->isDouble()
> +                                             ? MoveGroupResolver::Move::GENERAL
> +                                             : MoveGroupResolver::Move::DOUBLE;
> +
> +        if (!resolver.addMove(toMoveOperand(from), toMoveOperand(to), kind))

Having a boolean isDouble parameter seems cleaner to me than this "compute a Kind and pass it in" business.
Attachment #548123 - Flags: review?(adrake) → review+
Thanks for the quick review.

> can movePool_.allocate take the constructor args so we aren't doing
> placement new out here?

Hrm, that ends up being tricky, it's just a templated call.

> return pending_.insert(pm); ?

It's infallible.

> > +        Kind kind_ : 2;
> > +        uint32 code_ : 5;
> > +        int32 disp_;
> 
> This seems a bit premature at the moment, we should have a follow-up bug for
> shaving bits once we know which ones to shave.

Agree - removed.

> What benefit is gained from >= vs == here?

There was originally an extra address type - == works now.

> What's the difference between these two cases?

None; fixed.

> Microscopic Nit: Can the assert use from and to? I think it'd just look a
> little nicer.

Yup, worthy of a non-microscopic nit even.

> Having a boolean isDouble parameter seems cleaner to me than this "compute a
> Kind and pass it in" business.

I was kind of planning on someday having other types, like "int32" versus "machine word". I'm also against bools meaning weird things but so far I'm guilty of this all over the place :(
http://hg.mozilla.org/projects/ionmonkey/rev/a4c43d665f67
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.