Closed Bug 1258327 Opened 4 years ago Closed 4 years ago

Move ToOperand from CodeGeneratorShared to CodeGeneratorX86Shared.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: arai, Assigned: arai)

Details

Attachments

(8 files)

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.
[Part 1]

Moved CodeGeneratorMIPSShared::ToAddress to CodeGeneratorShared::ToAddress, to use it in generic CodeGenerator
Assignee: nobody → arai.unmht
Attachment #8733522 - Flags: review?(r)
[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)
[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)
[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)
[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)
[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)
[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 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 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+
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)
Attachment #8733527 - Flags: review?(jdemooij) → review+
Attachment #8733529 - Flags: review?(jdemooij) → review+
Attachment #8733523 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8733524 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8733532 - Flags: review?(nicolas.b.pierron) → review+
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)
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.
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.
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+
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
You need to log in before you can comment on or make changes to this bug.