Closed
Bug 1258327
Opened 8 years ago
Closed 8 years ago
Move ToOperand from CodeGeneratorShared to CodeGeneratorX86Shared.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
Details
Attachments
(8 files)
3.97 KB,
patch
|
hev
:
review+
|
Details | Diff | Splinter Review |
10.53 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
8.56 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
4.13 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
9.28 KB,
patch
|
hev
:
review+
|
Details | Diff | Splinter Review |
4.52 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
3.73 KB,
patch
|
hev
:
review+
|
Details | Diff | Splinter Review |
ToOperand is used only in a few places in generic CodeGenerator. After removing those callsites, we can remove Operand from generic macro assembler. Also, we can remove dummy ToOperand implementation from arm64.
Assignee | ||
Comment 1•8 years ago
|
||
[Part 1] Moved CodeGeneratorMIPSShared::ToAddress to CodeGeneratorShared::ToAddress, to use it in generic CodeGenerator
Assignee: nobody → arai.unmht
Attachment #8733522 -
Flags: review?(r)
Assignee | ||
Comment 2•8 years ago
|
||
[Part 2]
Replaced ToOperand call in following with ToRegister/ToAddress:
* CodeGenerator::visitBoundsCheck
* CodeGenerator::visitBoundsCheckRange
I believe float registers are not used there, as all input values/instructions are Int32. can you double check?
Then, both functions call bailoutCmp32, and bailoutCmp32 calls cmp32, so removed following:
* Operand variant of MacroAssemblerARMCompat::cmp32
* Operand variant of CodeGeneratorMIPSShared::bailoutCmp32
And added following:
* Address variant of MacroAssemblerARMCompat::cmp32
* Address variant of MacroAssemblerX86Shared::cmp32
In MacroAssemblerARMCompat::cmp32, only register Operand was supported
> void
> MacroAssemblerARMCompat::cmp32(const Operand& lhs, Register rhs)
> {
> ma_cmp(lhs.toReg(), rhs);
> }
so, I added Address variant with MOZ_CRASH, for now. (Looks like, memory Operand is used at least on x86 and x64, but not on arm and arm64. what could be the reason for this difference?)
Attachment #8733523 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•8 years ago
|
||
[Part 3] Replaced ToOperand call in following with ToRegister/ToAddress: * CodeGenerator::visitStoreTypedArrayElementHole Here too, can you double check that float register is not used? Also, removed following, as they're now not used. * Operand variant of MacroAssembler::branch32, except from x86-shared
Attachment #8733524 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 4•8 years ago
|
||
[Part 4] Replaced ToOperand call in following with ToRegister/ToAddress: * CodeGeneratorARM::visitCompare * CodeGeneratorARM::visitCompareAndBranch * CodeGeneratorARM::visitAddI * CodeGeneratorARM::visitSubI This might be a wrong approach, as this patch needs `Operand(ToAddress(foo))` as a replacement of `ToOperand(foo)`, but at least it clarifies that the Operand is always address.
Attachment #8733527 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•8 years ago
|
||
[Part 5] Just removed following: * CodeGeneratorARM64::ToOperand as ToOperand will be moved into x86-shared in Part 7, and there should be no need to override them with MOZ_CRASH impl.
Attachment #8733529 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•8 years ago
|
||
[Part 6] Replaced ToOperand call in following with ToRegister/ToAddress: * CodeGeneratorMIPS64::visitUnbox and replaced Operand variant of following with Register variant: * MacroAssemblerMIPS64Compat::unboxInt32 * MacroAssemblerMIPS64Compat::unboxBoolean * MacroAssemblerMIPS64Compat::unboxString * MacroAssemblerMIPS64Compat::unboxSymbol * MacroAssemblerMIPS64Compat::unboxObject
Attachment #8733531 -
Flags: review?(r)
Assignee | ||
Comment 7•8 years ago
|
||
[Part 7] Moved CodeGeneratorShared::ToOperand to CodeGeneratorX86Shared::ToOperand. Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82168a7c5aa4
Attachment #8733532 -
Flags: review?(nicolas.b.pierron)
Comment 8•8 years ago
|
||
Comment on attachment 8733522 [details] [diff] [review] Part 1: Move ToAddress from CodeGeneratorMIPSShared to CodeGeneratorShared. Review of attachment 8733522 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.
Attachment #8733522 -
Flags: review?(r) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8733531 [details] [diff] [review] Part 6: Remove ToOperand call from mips64 CodeGenerator. Review of attachment 8733531 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. :D
Attachment #8733531 -
Flags: review?(r) → review+
Comment 10•8 years ago
|
||
Would it make sense to add a new type in RegisterSets.h which does an union between an Int32 constant, a Register[1] and a stack-Address? In the current set of patches, I see that branches are already have 2 almost identical branches, and these patches are adding a third one: branch(cond, index, length, &label); This dispatch based on the type of the argument can be done as part of the MacroAssembler interface. Technically, what I propose, is just replacing Operand by another structure, what is important is that this other structure is not defined as part of the Assembler of each architecture. What do you think? [1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/RegisterSets.h?from=RegisterSets.h#292
Flags: needinfo?(arai.unmht)
Updated•8 years ago
|
Attachment #8733527 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8733529 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8733523 -
Flags: review?(nicolas.b.pierron) → review+
Updated•8 years ago
|
Attachment #8733524 -
Flags: review?(nicolas.b.pierron) → review+
Updated•8 years ago
|
Attachment #8733532 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Thank you for reviewing :D (In reply to Nicolas B. Pierron [:nbp] from comment #10) > Would it make sense to add a new type in RegisterSets.h which does an union > between an Int32 constant, a Register[1] and a stack-Address? > > In the current set of patches, I see that branches are already have 2 almost > identical branches, and these patches are adding a third one: > branch(cond, index, length, &label); > > This dispatch based on the type of the argument can be done as part of the > MacroAssembler interface. > > Technically, what I propose, is just replacing Operand by another structure, > what is important is that this other structure is not defined as part of the > Assembler of each architecture. > > What do you think? > > [1] > https://dxr.mozilla.org/mozilla-central/source/js/src/jit/RegisterSets. > h?from=RegisterSets.h#292 correct me if I'm mis-understanding the issue. As long as all types in the union are properly supported by each method, I think it's nice to either add new Operand-like type, or make one of existing Operand to generic. What I see with this current Operand thing is that, it's unclear that which actual type (generic register, float, address, baseindex, etc) is supported internally in certain method, thus it becomes the problem that what kind of type could flow into the method. (maybe, I should fix MOZ_CRASH("NYI") in cmp32...) I feel that is similar situation as using a template in generic MacroAssembler public method arguments. With union, of course, we could restrict the possible type that could flow into it. but I'm not sure if it's easy to handle if we want to add one more type to the union or make yet another union with different set of types.
Flags: needinfo?(arai.unmht)
Comment 12•8 years ago
|
||
I was thinking of having a different type, for a different union of accepted inputs. Operands are indeed too low level, and they do not have identical restrictions. This is not something we should rely on in the generic CodeGenerator as it is dependents on the platform. I guess the number of branches that we have today might not justify such new union of types. So, never mind, and we should see if this appear more frequently in the future.
Assignee | ||
Comment 13•8 years ago
|
||
ah, that makes sense :) if the conversion between 2 unions with different set of types can be done in simple syntax/code, it could be nice. "Small set to large set" conversion would happen frequently, as lower level function should accept larger set. "large set to small set" conversion might happen if one want to handle 2 types differently for some reason. That case should be done explicitly tho. anyway, I'll land these patches as is, for now.
Assignee | ||
Comment 14•8 years ago
|
||
Resolving conflict with bug 1258910.
Attachment #8734608 -
Flags: review?(r)
Comment 15•8 years ago
|
||
Comment on attachment 8734608 [details] [diff] [review] Part 6.1: Remove ToOperand call from mips-shared and mips64 CodeGenerator. Review of attachment 8734608 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.
Attachment #8734608 -
Flags: review?(r) → review+
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb67d75e2b0bc8f185e723ca0bf646dd68206e2 Bug 1258327 - Part 1: Move ToAddress from CodeGeneratorMIPSShared to CodeGeneratorShared. r=hev https://hg.mozilla.org/integration/mozilla-inbound/rev/67ba48bf0deda7d7f46e42b2336482231fef0c18 Bug 1258327 - Part 2: Remove Operand variant from bailoutCmp32. r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb79b64718a28a82586b0aab404c12d46b8c5d2 Bug 1258327 - Part 3: Remove Operand variant from branch32 except x86-shared. r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/4821a0156aba032851174e6f095eeb1437b2cea6 Bug 1258327 - Part 4: Remove ToOperand call from arm CodeGenerator. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/c73a466ab48c0ad8364bcc8a90a57caeae3c66ad Bug 1258327 - Part 5: Remove dummy ToOperand definition from arm64 CodeGenerator. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/bca4017b118375f338ceadc8ce3839bce0f9bec0 Bug 1258327 - Part 6: Remove ToOperand call from mips-shared and mips64 CodeGenerator. r=hev https://hg.mozilla.org/integration/mozilla-inbound/rev/3dcf36d71a610ea6f483f73750cf77702eefdd52 Bug 1258327 - Part 7: Move ToOperand from CodeGeneratorShared to CodeGeneratorX86Shared. r=nbp
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dfb67d75e2b0 https://hg.mozilla.org/mozilla-central/rev/67ba48bf0ded https://hg.mozilla.org/mozilla-central/rev/ebb79b64718a https://hg.mozilla.org/mozilla-central/rev/4821a0156aba https://hg.mozilla.org/mozilla-central/rev/c73a466ab48c https://hg.mozilla.org/mozilla-central/rev/bca4017b1183 https://hg.mozilla.org/mozilla-central/rev/3dcf36d71a61
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•