Closed
Bug 949668
Opened 10 years ago
Closed 10 years ago
Infrastructure for moves with type float32 and beyond
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(7 files)
34.95 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.68 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
22.87 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
27.07 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
871 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
49.17 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
18.46 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
This patch just tidies up the MoveOp (formerly MoveResolver::Move) class a little.
Attachment #8346796 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
This patch removes an unused isDouble() function.
Attachment #8346800 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8346796 -
Flags: review?(jdemooij) → review+
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8346800 -
Flags: review?(jdemooij) → review+
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
Backed out for Windows jit-test failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/f31913983745 https://tbpl.mozilla.org/php/getParsedLog.php?id=31942719&tree=Mozilla-Inbound
Assignee | ||
Comment 14•10 years ago
|
||
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]
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0f9522bcd259 https://hg.mozilla.org/mozilla-central/rev/0daa1bff4b16 https://hg.mozilla.org/mozilla-central/rev/92776efaabf4 https://hg.mozilla.org/mozilla-central/rev/b15245c461c1
Comment 16•10 years ago
|
||
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", ...
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cb4aa974a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/8754d98c1df4 https://hg.mozilla.org/integration/mozilla-inbound/rev/620f50eea597
Whiteboard: [leave open]
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/620f50eea597 https://hg.mozilla.org/mozilla-central/rev/8754d98c1df4 https://hg.mozilla.org/mozilla-central/rev/d3cb4aa974a7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•