Closed Bug 609898 Opened 12 years ago Closed 12 years ago

JM: TypeInference: do floating point register allocation


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bhackett1024, Assigned: bhackett1024)


(Blocks 1 open bug)



(3 files, 3 obsolete files)

JM should do register allocation for floating point variables and slots in a similar way to other values.  The problem up to now has been not knowing which variables/slots are floats; using a register for something which *could* be a float leads to lots of unnecessary syncing when that decision is wrong.

Using type inference (bug 557407), we can identify variables and stack slots that are definitely ints or floats.  With int -> float coercions in the right places (whenever pushing/storing an int or maybe-int to a float slot), we can ensure such values are always floats and keep them in FP registers.
I wrote a FPRegister allocator a while ago; it was rejected due to adding complexity, and I was told that we just would not be doing that. If things have changed since, that would make me very glad.
I think with inference things should be simpler.  Registers will only be allocated for things which are definitely floats, and there won't be some situation where a slot can have GPRs for its type and payload, and an FP register on top of that for its (possible) float value.  For slots which could be floats but are not known to be (int|float|undefined, say), the existing allocation strategy will be used (no FP register).
Attached patch WIP (obsolete) — Splinter Review
This adds floating point regalloc within basic blocks, also changes push() methods on FrameState to take information about known types.  Times:

function foo() {
  var x = 1.5;
  var y = 2.8;
  for (var i = 0; i < 10000000; i++)
    x += y;

v8          66ms
jsc         60ms
js -j       33ms
js -m (old) 64ms
js -m (new) 36ms
Assignee: general → bhackett1024
Attached patch WIP 2 (obsolete) — Splinter Review
This works on SS.  There are regressions in bitops-bitwise-and, bitops-nsieve-bits and crypto-aes which need to get fixed (there are values in these that are maybe-float but almost-always-int, and we'd be better off treating them as unknown than as definitely-float), 8-9ms improvement in other tests.
Attachment #488773 - Attachment is obsolete: true
Attached patch WIP 3 (obsolete) — Splinter Review
This works on SS without regressing anything.  Most of the regressions were due to tripping slow paths on bitop opcodes that handled maybe-double but not known-double (these have been fixed).  Running tests individually, my total speedup between this and bugs 604045 and 608750 is 34ms (12%, or 43ms on AWFY).
Attachment #488975 - Attachment is obsolete: true
Comment on attachment 489090 [details] [diff] [review]

Not looking for detailed implementation feedback (though it's welcome), but I'd like to make sure the changes this makes to the FrameState API look good.  Would like to get this into JM this week to dovetail with bug 609899.
Attachment #489090 - Flags: feedback?(sstangl)
Attachment #489090 - Flags: feedback?(dvander)
Attached patch patchSplinter Review
This fixes things so jit-tests work with (and without) inference.
Attachment #489090 - Attachment is obsolete: true
Attachment #489524 - Flags: feedback?(sstangl)
Attachment #489524 - Flags: feedback?(dvander)
Attachment #489090 - Flags: feedback?(sstangl)
Attachment #489090 - Flags: feedback?(dvander)
Attached patch uncopy patchSplinter Review
Fix for FrameState::uncopy on FP frame entries.
This unifies register allocation and management within FrameState for normal registers and FP registers.  Previously these were managed separately, leading to too much near-duplicate logic (and lots more near-duplicate logic in bug 609899).  This moves the contents of FPRegisters into Registers, which can now represent masks containing both normal and FP registers, represented by the type AnyRegisterID.
Blocks: 619423
No longer blocks: TypeInference
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #489524 - Flags: feedback?(sstangl)
You need to log in before you can comment on or make changes to this bug.