Closed Bug 1112563 Opened 5 years ago Closed 5 years ago

Use useRegisterAtStart for LoadSlot/LoadFixedSlot/GetDOMMember

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
The main worry is when we load a Value on 32-bit platforms (we emit two loads and we don't want the first one to clobber our object/slots register), but fortunately masm.loadValue already knows how to deal with this (it just swaps the loads) so this should be safe.
Attachment #8537809 - Flags: review?(sunfish)
Comment on attachment 8537809 [details] [diff] [review]
Patch

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

Looks good. Can LStoreFixedSlotV and friends use AtStart too?
Attachment #8537809 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc612739ad7

(In reply to Dan Gohman [:sunfish] from comment #1)
> Can LStoreFixedSlotV and friends use AtStart too?

I think so, but what'd be the benefit if they have no defs/temps? We've been pretty conservative with *AtStart to do the safe thing by default..
https://hg.mozilla.org/mozilla-central/rev/cfc612739ad7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attached patch Follow-up fixSplinter Review
bz noticed that codegen is slightly worse on x64 when we're loading an object and reusing the input register (extra move to scratch register). This patch fixes that by using useRegister in this case. x64 has more registers so the optimization is less useful there anyway.
Attachment #8538493 - Flags: review?(sunfish)
Comment on attachment 8538493 [details] [diff] [review]
Follow-up fix

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

Makes sense.
Attachment #8538493 - Flags: review?(sunfish) → review+
Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f5b3825bbb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/e4f5b3825bbb
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.