Closed Bug 949668 Opened 6 years ago Closed 6 years ago

Infrastructure for moves with type float32 and beyond

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(7 files)

While discussing bug 947711 and bug 948853, we noticed that the move resolver doesn't currently distinguish between float32 and double. It would be nice to fix this, and fixing this will also make it easier to add support for SIMD types.

Attached is a series of patch which implements adding type information to LMove, and adding a float32 type for it.

This is a slightly different approach than what we discussed previously, and I apologize for the churn and merge conflicts :-(. This patch series adds a FLOAT32 type to LDefinition, and does not add a FLOAT32 kind to LAllocation. I now think this is a better approach, since LAllocation::Kind is more concerned with which register or stack slot something is in, and LDefinition::Type is more concerned with the interpretation of the value.

Further work is needed, but this patch series should be enough to demonstrate the idea, get checked in (assuming reviews are positive), and serve as a base for further work.
This patch just moves MoveResolver::Move and MoveResolver::MoveOperand to namespace scope, and renames Move to MoveOp to avoid conflicts with mozilla::Move.

This eliminates the need for a bunch of typedefs like:

    typedef MoveResolver::MoveOperand MoveOperand;
    typedef MoveResolver::Move Move;

which invites conflicts with "using mozilla::Move" and which is annoying to add to every bit of code that wants to work with the MoveResolver.
Attachment #8346794 - Flags: review?(jdemooij)
This patch just tidies up the MoveOp (formerly MoveResolver::Move) class a little.
Attachment #8346796 - Flags: review?(jdemooij)
This patch adds a FLOAT32 kind to LDefinition::Type.

Note that it doesn't do much yet; float32 still uses 64-bit moves and stack slots, but this is a step toward changing that.
Attachment #8346797 - Flags: review?(jdemooij)
Attached patch lmove-type.patchSplinter Review
This adds an LDefinition::Type to LMove, which will carry type information to the MoveResolver, and teaches all the places that create LMoves to supply a type.
Attachment #8346798 - Flags: review?(jdemooij)
This patch removes an unused isDouble() function.
Attachment #8346800 - Flags: review?(jdemooij)
This patch eliminates the various MacroAssembler-*.h Result enums, and replaces then with MoveOp::Kind.

It consolidates MoveOperand's ADDRESS and FLOAT_ADDRESS into a single value, and renames it to MEMORY, to help differentiate it from EFFECTIVE_ADDRESS.

It also eliminates MoveOperand::AddressKind and just uses MoveOperand::Kind, for simplicity.
Attachment #8346804 - Flags: review?(jdemooij)
This patch adds a FLOAT32 to MoveOp::Kind.

Note that this doesn't actually implement using movss instead of movsd for FLOAT32 on x86/x64, but that could easily be done with the mechanism in this patch.
Attachment #8346807 - Flags: review?(jdemooij)
Blocks: 904918
Blocks: 931434
Comment on attachment 8346794 [details] [diff] [review]
move-rename.patch

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

Nice cleanup, thanks.

::: js/src/jit/CodeGenerator.cpp
@@ +1184,5 @@
>          JS_ASSERT(from->isDouble() == to->isDouble());
>  
> +        MoveOp::Kind kind = from->isDouble()
> +                            ? MoveOp::DOUBLE
> +                            : MoveOp::GENERAL;

Nit: this should fit on one line now.
Attachment #8346794 - Flags: review?(jdemooij) → review+
Attachment #8346796 - Flags: review?(jdemooij) → review+
Comment on attachment 8346797 [details] [diff] [review]
ldefinition-float32.patch

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

::: js/src/jit/RegisterAllocator.h
@@ +368,5 @@
>  
>  static inline AnyRegister
>  GetFixedRegister(const LDefinition *def, const LUse *use)
>  {
> +    return def->type() == LDefinition::DOUBLE || def->type() == LDefinition::FLOAT32

Nit: add parentheses around this ||, so that I don't have to think about operator precedence.

::: js/src/jit/StupidAllocator.cpp
@@ +163,5 @@
>  
>      for (size_t i = 0; i < registerCount; i++) {
>          AnyRegister reg = registers[i].reg;
>  
> +        if (reg.isFloat() != (def->type() == LDefinition::DOUBLE || def->type() == LDefinition::FLOAT32))

Nit: this "== double or float32" check appears in some other places, maybe we could add a method to LDefinition, or a static function to which we can pass LDefinition::Type?
Attachment #8346797 - Flags: review?(jdemooij) → review+
Comment on attachment 8346798 [details] [diff] [review]
lmove-type.patch

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

We can use the same infrastructure to emit movl instead of movq for reg <-> memory moves of 32-bit values on x64, right? Do you think that's worth doing? Maybe we can also use 32-bit stack slots.

::: js/src/jit/LinearScan.cpp
@@ +298,5 @@
>      return true;
>  }
>  
>  bool
> +LinearScanAllocator::moveInputAlloc(CodePosition pos, LAllocation *from, LAllocation *to, LDefinition::Type type)

Nit: > 99 chars, so wrap like this:

LinearScanAllocator::moveInputAlloc(CodePosition pos, LAllocation *from, LAllocation *to,
                                    LDefinition::Type)
{
    ...
Attachment #8346798 - Flags: review?(jdemooij) → review+
Attachment #8346800 - Flags: review?(jdemooij) → review+
Comment on attachment 8346804 [details] [diff] [review]
enum-simplification.patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +4023,5 @@
>      FloatRegister input = ToFloatRegister(ins->input());
>      JS_ASSERT(ToFloatRegister(ins->output()) == ReturnFloatReg);
>  
>      masm.setupUnalignedABICall(1, temp);
> +    masm.passABIArg(input, MoveOp::DOUBLE);

I wanted to say MoveOp::FLOAT32, but I see your next patch does this.

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +598,4 @@
>                  argDisp += sizeof(void *);
>                  break;
>                case VMFunction::WordByRef:
> +                masm.passABIArg(MoveOperand(argsBase, argDisp, MoveOperand::EFFECTIVE_ADDRESS), MoveOp::GENERAL);

Nit: > 99 chars.

Pre-existing, but while you're here, can you remove the outer "if (f.explicitArgs) {"? It's redundant with the "explicitArg < f.explicitArgs" loop condition. Same for Trampoline-x86.cpp. Trampoline-arm.cpp doesn't have it either.

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +631,4 @@
>                  argDisp += sizeof(void *);
>                  break;
>                case VMFunction::WordByRef:
> +                masm.passABIArg(MoveOperand(argsBase, argDisp, MoveOperand::EFFECTIVE_ADDRESS), MoveOp::GENERAL);

Nit: > 99 chars

@@ +635,4 @@
>                  argDisp += sizeof(void *);
>                  break;
>                case VMFunction::DoubleByRef:
> +                masm.passABIArg(MoveOperand(argsBase, argDisp, MoveOperand::EFFECTIVE_ADDRESS), MoveOp::GENERAL);

And here.
Attachment #8346804 - Flags: review?(jdemooij) → review+
Comment on attachment 8346807 [details] [diff] [review]
moveop-float32.patch

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

Looks great!

::: js/src/jit/arm/Assembler-arm.h
@@ +2158,5 @@
>  
>  static inline uint32_t
> +GetFloat32ArgStackDisp(uint32_t usedIntArgs, uint32_t usedFloatArgs, uint32_t *padding)
> +{
> +

Nit: remove blank line.

@@ +2162,5 @@
> +
> +    JS_ASSERT(usedFloatArgs >= NumFloatArgRegs);
> +    uint32_t intSlots = 0;
> +    if (usedIntArgs > NumIntArgRegs) {
> +        intSlots = usedIntArgs - NumIntArgRegs;

Nit: no {}

@@ +2174,2 @@
>  {
>  

Pre-existing nit: remove blank line.
Attachment #8346807 - Flags: review?(jdemooij) → review+
Relanding the first 4 patches:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9522bcd259
https://hg.mozilla.org/integration/mozilla-inbound/rev/0daa1bff4b16
https://hg.mozilla.org/integration/mozilla-inbound/rev/92776efaabf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15245c461c1

This leaves enum-simplification.patch and moveop-float32.patch unapplied, until I can fix the win32 failures.

(In reply to Jan de Mooij [:jandem] from comment #9)
> @@ +163,5 @@
> >  
> >      for (size_t i = 0; i < registerCount; i++) {
> >          AnyRegister reg = registers[i].reg;
> >  
> > +        if (reg.isFloat() != (def->type() == LDefinition::DOUBLE || def->type() == LDefinition::FLOAT32))
> 
> Nit: this "== double or float32" check appears in some other places, maybe
> we could add a method to LDefinition, or a static function to which we can
> pass LDefinition::Type?

Yes, I think isFloatRegClass() would be good to add.

(In reply to Jan de Mooij [:jandem] from comment #10)
> We can use the same infrastructure to emit movl instead of movq for reg <->
> memory moves of 32-bit values on x64, right? Do you think that's worth
> doing? Maybe we can also use 32-bit stack slots.

movl is one byte smaller than movq :-). I think we can do both, once this infrastructure is in place.
Whiteboard: [leave open]
Blocks: 950703
Comment on attachment 8346797 [details] [diff] [review]
ldefinition-float32.patch

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

::: js/src/jit/LIR.cpp
@@ +195,5 @@
>  static const char * const TypeChars[] =
>  {
>      "i",            // INTEGER
>      "o",            // OBJECT
> +    "f",            // FLOAT32

It seems that SLOTS is missed. I suggest:
   "i",
   "o",
   "s",
   "f",
...
Depends on: 956606
Depends on: 957504
You need to log in before you can comment on or make changes to this bug.