Closed
Bug 1245112
Opened 9 years ago
Closed 9 years ago
Move MacroAssembler branch functions to the generic macro assembler.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(40 files, 4 obsolete files)
just like bug 1203964 and bug 1229057, Move declarations of branch related functions to the generic MacroAssembler.
branch32
branchPtr
branchPtrWithPatch
branchPtrInNurseryRange
branchPrivatePtr
branchFloat
branchTruncateFloat32
branchDouble
branchTruncateDouble
branchAdd32
branchSub32
decBranchPtr
branchNegativeZero
branchNegativeZeroFloat32
branchTest16
branchTest32
branchTest64
branchTestPtr
branchTestInt32
branchTestInt32Truthy
branchTestDouble
branchTestDoubleTruthy
branchTestNumber
branchTestBoolean
branchTestBooleanTruthy
branchTestString
branchTestStringTruthy
branchTestSymbol
branchTestUndefined
branchTestNull
branchTestPrimitive
branchTestMagic
branchTestMagicValue
branchTestGCThing
branchTestObject
branchTestValue
branchValueIsNurseryObject
Assignee | ||
Comment 1•9 years ago
|
||
will post some basic parts after test.
Assignee: nobody → arai.unmht
status-firefox47:
affected → ---
Assignee | ||
Comment 2•9 years ago
|
||
as a first step, prepared 10 patches for some basic methods and depending things.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a681aefa1f
Attachment #8716641 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8716642 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8716643 -
Flags: review?(nicolas.b.pierron)
Comment 5•9 years ago
|
||
Comment on attachment 8716641 [details] [diff] [review]
Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler.
Review of attachment 8716641 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for looking at this technical debt. :)
As experienced, differences between platforms can highlight bugs in any platform. Thus we should take this opportunity to question the differences and to comment them.
::: js/src/jit/MacroAssembler.h
@@ +807,5 @@
>
> + // ===============================================================
> + // Branch functions
> +
> + inline void branchPrivatePtr(Condition cond, Register lhs, ImmWord rhs, Label* label)
Remove this unused function.
@@ +811,5 @@
> + inline void branchPrivatePtr(Condition cond, Register lhs, ImmWord rhs, Label* label)
> + DEFINED_ON(arm, arm64, mips_shared);
> + inline void branchPrivatePtr(Condition cond, const Address& lhs, Register rhs, Label* label)
> + DEFINED_ON(arm, arm64, mips_shared, x86, x64);
> + inline void branchPrivatePtr(Condition cond, const Address& lhs, ImmPtr rhs, Label* label)
Remove this unused function.
::: js/src/jit/x64/MacroAssembler-x64-inl.h
@@ +227,5 @@
> +{
> + ScratchRegisterScope scratch(*this);
> + if (rhs != scratch)
> + movePtr(rhs, scratch);
> + rshiftPtr(Imm32(1), scratch);
This code got added in Bug 856627 without any comment on why this is different from other architecture, such as x86 and arm.
As we added new backends since, such as mips32, mips64 and arm64, I definitely want to know if this is an optimization based on the word size or not.
I will cancel this review until we can figure this out, and add a proper comment and/or fix the code accordingly.
From what I can read, I cannot see any reason to shift this value by 1, the pointers does not seems to be boxed in any way. This function is called from within the SharedIC variant of CheckDOMProxyExpandoDoesNotShadow, while the IonCache variant of it does not do that.
Looking at how the baseline IC is initialized let me think that this shift has no reason to exists, and thus that we can replace the branchPrivatePtr by a simple branchPtr.
=> ni? efaust, as this is related to DOM proxy code.
Attachment #8716641 -
Flags: review?(nicolas.b.pierron) → feedback+
Comment 6•9 years ago
|
||
Eric, can you look at the Part 1, and tell us if this right shift is something that we should keep?
Flags: needinfo?(efaustbmo)
Comment 7•9 years ago
|
||
Comment on attachment 8716642 [details] [diff] [review]
Part 2: Move MacroAssembler::branchPtr into generic macro assembler.
Review of attachment 8716642 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +428,5 @@
> +void
> +MacroAssembler::branchPtr(Condition cond, Register lhs, wasm::SymbolicAddress rhs, Label* label)
> +{
> + ScratchRegisterScope scratch(*this);
> + movePtr(rhs, scratch);
This sounds like an existing bug in the arm & mips-shared backend implementations, as
branchPtr(Condition, Register, wasm::SymbolicAddress, Label*)
uses movePtr
and
branchPtr(Condition, wasm::SymbolicAddress, Register, Label*)
uses loadPtr
=> ni? bbouvier, luke
@@ +448,5 @@
> +void
> +MacroAssembler::branchPtr(Condition cond, const Address& lhs, ImmGCPtr rhs, Label* label)
> +{
> + AutoRegisterScope scratch2(*this, secondScratchReg_);
> + ma_ldr(lhs, scratch2);
nit: use loadPtr instead of ma_ldr, and do the same for the functions which is below.
@@ +450,5 @@
> +{
> + AutoRegisterScope scratch2(*this, secondScratchReg_);
> + ma_ldr(lhs, scratch2);
> + ma_cmp(scratch2, rhs);
> + ma_b(label, cond);
nit: replace these 2 instructions by a call to
branchPtr(cond, scratch2, rhs, label);
And do the same for the branchPtr functions which are below.
::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +508,5 @@
> +
> + movePtr(rhs, scratch1_64.asUnsized());
> + loadPtr(lhs, scratch2_64.asUnsized());
> + cmp(scratch2_64, scratch1_64);
> + B(cond, label);
existing-nit: use branchPtr here, instead of cmp & B functions.
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +227,5 @@
> +void
> +MacroAssembler::branchPtr(Condition cond, Register lhs, wasm::SymbolicAddress rhs, Label* label)
> +{
> + movePtr(rhs, SecondScratchReg);
> + ma_b(lhs, SecondScratchReg, label, cond);
existing-nit: As other platform implementation, I will suggest to use branchPtr is there is already a movePtr / loadPtr in the body of the function.
::: js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ +244,5 @@
> +template <typename T, typename S>
> +void
> +MacroAssemblerX86Shared::branchPtrImpl(Condition cond, const T& lhs, const S& rhs, Label* label)
> +{
> + asMasm().cmpPtr(Operand(lhs), rhs);
self-note: Operand variant of cmpPtr belongs to MacroAssembler{X86,X64}, and thus the asMasm() trick is just a way to reuse the inheritance. Operand variants do not belong to the generic MacroAssembler because Operands are only specified per-architecture.
nit: Maybe we should copy this branchPtrImpl in both MacroAssemblerX86 and MacroAssemblerX64? Or Maybe we could define the missing cmpPtr variants on the generic MacroAssembler, and not use the Operand(…) constructor.
::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +272,5 @@
> +
> +void
> +MacroAssembler::branchPtr(Condition cond, wasm::SymbolicAddress lhs, Register rhs, Label* label)
> +{
> + cmpl(rhs, lhs);
existing-nit: Note that operands are swapped! This implies that either the condition flag should be swapped to or an assertion which ensure that this function is only called with
cond == Equal || cond == NotEqual.
ni? bbouvier, luke
Attachment #8716642 -
Flags: review?(nicolas.b.pierron) → review+
Comment 8•9 years ago
|
||
Benjamin and Luke, can you look at part 2 comments, and give some feedback about the branchPtr implementation with wasm::SymbolicAddress, both on arm & mips-shared, and also on x86-inl and x64-inl.
Flags: needinfo?(luke)
Flags: needinfo?(bbouvier)
Comment 9•9 years ago
|
||
I could be reading this wrong but it appears that, in Assembler-x86.h, all the cmpl overloads have lhs and rhs in the wrong order and the SymbolicAddress overloads is just copying that.
Updated•9 years ago
|
Flags: needinfo?(luke)
Comment 10•9 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #9)
> I could be reading this wrong but it appears that, in Assembler-x86.h, all
> the cmpl overloads have lhs and rhs in the wrong order and the
> SymbolicAddress overloads is just copying that.
Oh, thanks, I did not notice that this was cmpl and not cmpPtr, as all others.
I guess this is one more reasons for in favor of the generic MacroAssembler.
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 11•9 years ago
|
||
what about loadPtr vs movePtr in arm & mips-shared "Register lhs, wasm::SymbolicAddress rhs" cases in comment #7 ?
Flags: needinfo?(luke)
Comment 12•9 years ago
|
||
Oops, sorry. It looks like the movePtr variants are dead code from back when we had two types (to distinguish immediates (that you'd move) and addresses (that you'd load). Now there is just SymbolicAddress and it means "load".
Flags: needinfo?(luke)
Comment 13•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> From what I can read, I cannot see any reason to shift this value by 1, the
> pointers does not seems to be boxed in any way. This function is called
> from within the SharedIC variant of CheckDOMProxyExpandoDoesNotShadow, while
> the IonCache variant of it does not do that.
> => ni? efaust, as this is related to DOM proxy code.
So, what this function does is takes an Address to a PrivateValue, and a void* and compares them. Rather than loadValue and unboxPrivate and branchPtr, it instead "boxes" the void* in a scratch register, and does a direct comparison with the PrivateValue. Since this process, on 64bits is a rshift, that's what we do. I admit that it's open-coded, but this seems like a fairly straightforward implementation of the function. I don't know what "keep" means in this context. Unifying it against others seems hard, since this is a 64-bit specific handling, but the rshift is totally mandatory for the correctness of branchPrivatePtr there.
Flags: needinfo?(efaustbmo)
Comment 14•9 years ago
|
||
(In reply to Eric Faust [:efaust] from comment #13)
> I don't know what "keep" means in this context.
OK, Nicolas has explained more context to me. The difference in the callers is warranted: in baseline (SharedStubs), the PrivateValue is stored in the stub, so it's read by the jitcode, and both sides of the check are "dynamic". In Ion, we bake the private pointer directly into the stub's jitcode, so it's "statically" known. That's why the code snippets are different.
We can /still/ maybe remove this function, by just storing a void* in the stub, rather than a PrivateValue that we need to unbox. Maybe it's worth looking in to this.
Either way, Nicolas points out that we should have such a shift for all PUNBOX_64 architectures, which probably means fixing ARM64 and maybe MIPS64.
Assignee | ||
Comment 15•9 years ago
|
||
Removed unused variants, splitted mips-shared into 32/64, added comment for x64, and applied same fix to arm64/mips64.
Attachment #8716641 -
Attachment is obsolete: true
Attachment #8717772 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> (In reply to Luke Wagner [:luke] from comment #9)
> > I could be reading this wrong but it appears that, in Assembler-x86.h, all
> > the cmpl overloads have lhs and rhs in the wrong order and the
> > SymbolicAddress overloads is just copying that.
>
> Oh, thanks, I did not notice that this was cmpl and not cmpPtr, as all
> others.
> I guess this is one more reasons for in favor of the generic MacroAssembler.
so, should we swap the arguments of cmpl definition?
(actually I'm not sure which part of assembler/macro assembler follows which syntax or operand order...)
Flags: needinfo?(nicolas.b.pierron)
Comment 17•9 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #16)
> (In reply to Nicolas B. Pierron [:nbp] from comment #10)
> > (In reply to Luke Wagner [:luke] from comment #9)
> > > I could be reading this wrong but it appears that, in Assembler-x86.h, all
> > > the cmpl overloads have lhs and rhs in the wrong order and the
> > > SymbolicAddress overloads is just copying that.
> >
> > Oh, thanks, I did not notice that this was cmpl and not cmpPtr, as all
> > others.
> > I guess this is one more reasons for in favor of the generic MacroAssembler.
>
> so, should we swap the arguments of cmpl definition?
No cmpl is effectively taking its arguments in reversed order as opposed to cmpPtr. It attempts to follow the way the assembler syntax is written [1].
The only case where the arguments seems to be accidentally switched does not matter, as we are checking for NotEqual condition. [2]
[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x86-shared/BaseAssembler-x86-shared.h#1400
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp#1099
Flags: needinfo?(nicolas.b.pierron)
Comment 18•9 years ago
|
||
Comment on attachment 8717772 [details] [diff] [review]
Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler.
Review of attachment 8717772 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks :)
::: js/src/jit/MacroAssembler.h
@@ +800,5 @@
>
> + // ===============================================================
> + // Branch functions
> +
> + inline void branchPrivatePtr(Condition cond, const Address& lhs, Register rhs, Label* label) PER_ARCH;
I think it is important to comment above this function, as both inputs have different types:
// This function compares a Value (lhs) which is having a private pointer boxed
// inside a js::Value, with a raw pointer (rhs).
Attachment #8717772 -
Flags: review?(nicolas.b.pierron) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8716643 [details] [diff] [review]
Part 3: Move MacroAssembler::branch32 into generic macro assembler.
Review of attachment 8716643 [details] [diff] [review]:
-----------------------------------------------------------------
Apart from existing issues in the arm variant of the branch32 comments, which deserves its own bug. I do not see any issue with this patch.
::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +482,5 @@
> +{
> + ScratchRegisterScope scratch(*this);
> + loadPtr(lhs, scratch);
> + ma_cmp(scratch, rhs);
> + ma_b(label, cond);
nit: use branch32(cond, scratch, rhs, label);
Attachment #8716643 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 20•9 years ago
|
||
fixed following:
* replace masm.asmOnOutOfBoundsLabel() with wasm::JumpTarget::OutOfBounds
* templatize
* MacroAssemblerMIPSShared::ma_b
* MacroAssemblerMIPSCompat::branchTestMagic
* MacroAssemblerMIPSCompat::jump
* MacroAssemblerMIPSCompat::branchTest32
* MacroAssemblerMIPS64Compat::branchTestMagic
* MacroAssemblerMIPS64Compat::jump
* MacroAssemblerMIPS64Compat::branchTest32
* add
* MacroAssemblerMIPS::branchWithCode
* MacroAssemblerMIPS64::branchWithCode
* move MoveEmitterMIPSShared::MoveEmitterMIPSShared to cpp, as framePushed is not yet defined.
as AssemblerMIPSShared::bindLater is not-yet-implemented, MacroAssemblerMIPS::branchWithCode is also not-yet-implemented.
Attachment #8718233 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 21•9 years ago
|
||
branchTest64 was used in previous Math.random impl, and looks like it's no more used.
in this patch I moved it to MacroAssembler, but maybe it's better just remove it?
Attachment #8718234 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 22•9 years ago
|
||
x86 and x64 have exactly same impl, so merged them in x86-shared.
also, mips32 and mips64 have exactly same impl, so merged them in mips-shared.
Assignee | ||
Updated•9 years ago
|
Attachment #8718235 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 23•9 years ago
|
||
in x86-shared, Operand variant seems not to be used, so removed.
mips32 and mips64 have exactly same impl, metged and applied template (but it's not buildable, as it does not support js::wasm::JumpTarget).
Attachment #8718236 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8718282 -
Flags: review?(nicolas.b.pierron)
Comment 25•9 years ago
|
||
Comment on attachment 8718233 [details] [diff] [review]
Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
Review of attachment 8718233 [details] [diff] [review]:
-----------------------------------------------------------------
I will forward the review to our MIPS maintainers.
Attachment #8718233 -
Flags: review?(r)
Attachment #8718233 -
Flags: review?(nicolas.b.pierron)
Attachment #8718233 -
Flags: review?(branislav.rankov)
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8718283 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8718285 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8718286 -
Flags: review?(nicolas.b.pierron)
Comment 29•9 years ago
|
||
Comment on attachment 8718234 [details] [diff] [review]
Part 4: Move MacroAssembler::branchTest64 into generic macro assembler.
Review of attachment 8718234 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +582,5 @@
> + if (cond == Assembler::Zero) {
> + MOZ_ASSERT(lhs.low == rhs.low);
> + MOZ_ASSERT(lhs.high == rhs.high);
> + mov(lhs.low, ScratchRegister);
> + or32(lhs.high, ScratchRegister);
Note: Like mips, ARM has a 3 way or instruction:
ma_orr(lhs.low, lhs.high, ScratchRegister);
Attachment #8718234 -
Flags: review?(nicolas.b.pierron) → review+
Updated•9 years ago
|
Attachment #8718235 -
Flags: review?(nicolas.b.pierron) → review+
Comment 30•9 years ago
|
||
Comment on attachment 8718236 [details] [diff] [review]
Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
Review of attachment 8718236 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +689,5 @@
> +MacroAssembler::branchTest32(Condition cond, const AbsoluteAddress& lhs, Imm32 rhs, Label* label)
> +{
> + vixl::UseScratchRegisterScope temps(this);
> + const Register scratch = temps.AcquireX().asUnsized();
> + loadPtr(lhs, scratch);
=> ni? sstangl: Shouldn't this be a load32 instead of a loadPtr.
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +350,5 @@
> +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> +{
> + MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> + if (lhs == rhs) {
> + ma_b(lhs, rhs, label, cond);
This sounds busted, as this is the same implementation for branchTest64 on mips64.
=> ni? Heiher
Comment 31•9 years ago
|
||
Heiher, Sean, can you have a look at the review in comment 30?
Flags: needinfo?(sstangl)
Flags: needinfo?(r)
Updated•9 years ago
|
Attachment #8718236 -
Flags: review?(nicolas.b.pierron) → review+
Comment 32•9 years ago
|
||
Comment on attachment 8718282 [details] [diff] [review]
Part 7: Move MacroAssembler::branchFloat into generic macro assembler.
Review of attachment 8718282 [details] [diff] [review]:
-----------------------------------------------------------------
Forward the review to Benjamin.
Attachment #8718282 -
Flags: review?(nicolas.b.pierron) → review?(bbouvier)
Comment 33•9 years ago
|
||
Comment on attachment 8718283 [details] [diff] [review]
Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler.
Review of attachment 8718283 [details] [diff] [review]:
-----------------------------------------------------------------
Forward the review to Benjamin.
Attachment #8718283 -
Flags: review?(nicolas.b.pierron) → review?(bbouvier)
Comment 34•9 years ago
|
||
Comment on attachment 8718236 [details] [diff] [review]
Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
Review of attachment 8718236 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +689,5 @@
> +MacroAssembler::branchTest32(Condition cond, const AbsoluteAddress& lhs, Imm32 rhs, Label* label)
> +{
> + vixl::UseScratchRegisterScope temps(this);
> + const Register scratch = temps.AcquireX().asUnsized();
> + loadPtr(lhs, scratch);
Yes, this should be a load32.
Updated•9 years ago
|
Flags: needinfo?(sstangl)
Comment 35•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #30)
> Comment on attachment 8718236 [details] [diff] [review]
> Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
>
> Review of attachment 8718236 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> @@ +350,5 @@
> > +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> > +{
> > + MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> > + if (lhs == rhs) {
> > + ma_b(lhs, rhs, label, cond);
>
> This sounds busted, as this is the same implementation for branchTest64 on
> mips64.
>
> => ni? Heiher
I think yes, The 32-bit integers are always sign-extend into 64-bit register on mips64.
Flags: needinfo?(r)
Comment 36•9 years ago
|
||
(In reply to Heiher [:hev] from comment #35)
> (In reply to Nicolas B. Pierron [:nbp] from comment #30)
> > Comment on attachment 8718236 [details] [diff] [review]
> > Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
> >
> > Review of attachment 8718236 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> > @@ +350,5 @@
> > > +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> > > +{
> > > + MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> > > + if (lhs == rhs) {
> > > + ma_b(lhs, rhs, label, cond);
> >
> > This sounds busted, as this is the same implementation for branchTest64 on
> > mips64.
> >
> > => ni? Heiher
>
> I think yes, The 32-bit integers are always sign-extend into 64-bit register
> on mips64.
So that means that if we are trying to compare the low bits of a 64bits value, with a 32bits immediate, then we might have an issue where the test will not work as expected?
Should we neuter the high bits of the other register then?
Updated•9 years ago
|
Attachment #8718285 -
Flags: review?(nicolas.b.pierron) → review+
Comment 37•9 years ago
|
||
Comment on attachment 8718286 [details] [diff] [review]
Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler.
Review of attachment 8718286 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +727,5 @@
> + ARMRegister dest64(dest, 64);
> +
> + MOZ_ASSERT(!scratch64.Is(dest64));
> +
> + //breakpoint();
nit: remove this debug-comment.
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +211,5 @@
> +// was clamped to INT32_MIN/INT32_MAX, and we can test it.
> +// NOTE: if the value really was supposed to be INT32_MAX / INT32_MIN then it
> +// will be wrong.
> +void
> +MacroAssembler::branchTruncateDouble(FloatRegister src, Register dest, Label* fail)
This function is identical to MIPS32 implementation, move it into MacroAssembler-mips-shared-inl.h
Attachment #8718286 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 38•9 years ago
|
||
merged mips32/64.
Attachment #8718283 -
Attachment is obsolete: true
Attachment #8718283 -
Flags: review?(bbouvier)
Attachment #8719128 -
Flags: review?(bbouvier)
Assignee | ||
Comment 39•9 years ago
|
||
Moved following methods
* branchIfFalseBool
* branchIfTrueBool
* branchIfRope
* branchLatin1String
* branchTwoByteString
* branchIfFunctionHasNoScript
* branchIfInterpreted
* branchFunctionKind
* branchIfNotInterpretedConstructor
* branchTestObjClass
* branchTestObjShape
* branchTestObjGroup
* branchTestObjectTruthy
* branchTestClassIsProxy
* branchTestObjectIsProxy
* branchTestProxyHandlerFamily
* branchTestMIRType
* branchEqualTypeIfNeeded
* branchKey
* branchTestNeedsIncrementalBarrier
Attachment #8719129 -
Flags: review?(nicolas.b.pierron)
Comment 40•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #36)
> (In reply to Heiher [:hev] from comment #35)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #30)
> > > Comment on attachment 8718236 [details] [diff] [review]
> > > Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
> > >
> > > Review of attachment 8718236 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > > ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> > > @@ +350,5 @@
> > > > +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> > > > +{
> > > > + MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> > > > + if (lhs == rhs) {
> > > > + ma_b(lhs, rhs, label, cond);
> > >
> > > This sounds busted, as this is the same implementation for branchTest64 on
> > > mips64.
> > >
> > > => ni? Heiher
> >
> > I think yes, The 32-bit integers are always sign-extend into 64-bit register
> > on mips64.
>
> So that means that if we are trying to compare the low bits of a 64bits
> value, with a 32bits immediate, then we might have an issue where the test
> will not work as expected?
>
> Should we neuter the high bits of the other register then?
Yes. If one operand of branchTest32 is a real 64bits value, we need neuter the high bits of this value.
Comment 41•9 years ago
|
||
Comment on attachment 8718233 [details] [diff] [review]
Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
Review of attachment 8718233 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you fix, link failed:
Executing: mips64el-unknown-linux-gnu-g++ -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wtype-limits -Wcast-align -Wno-invalid-offsetof -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -g -O3 -march=loongson3a -fdiagnostics-color=auto -mno-branch-likely -w -fomit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,-h,libmozjs-47a1.so -o libmozjs-47a1.so /home/heiher/git/mips64/gecko-dev/js/src/mips64-release/js/src/tmpcr__PD.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -Wl,-version-script,symverscript -Wl,-rpath-link,/home/heiher/git/mips64/gecko-dev/js/src/mips64-release/dist/bin -Wl,-rpath-link,/usr/lib -lm -ldl -L/opt/mips64el-toolchain/platforms/current/usr/lib64 -licui18n -licuuc -licudata -L/usr/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl -lz -lm
/home/heiher/git/mips64/gecko-dev/js/src/mips64-release/js/src/tmpcr__PD.list:
INPUT("RegExp.o")
INPUT("Parser.o")
INPUT("StoreBuffer.o")
INPUT("ExecutableAllocatorPosix.o")
INPUT("jsarray.o")
INPUT("jsatom.o")
INPUT("jsdtoa.o")
INPUT("jsmath.o")
INPUT("jsutil.o")
INPUT("pm_linux.o")
INPUT("Initialization.o")
INPUT("TraceLogging.o")
INPUT("TraceLoggingGraph.o")
INPUT("TraceLoggingTypes.o")
INPUT("Unified_cpp_js_src0.o")
INPUT("Unified_cpp_js_src1.o")
INPUT("Unified_cpp_js_src10.o")
INPUT("Unified_cpp_js_src11.o")
INPUT("Unified_cpp_js_src12.o")
INPUT("Unified_cpp_js_src13.o")
INPUT("Unified_cpp_js_src14.o")
INPUT("Unified_cpp_js_src15.o")
INPUT("Unified_cpp_js_src16.o")
INPUT("Unified_cpp_js_src17.o")
INPUT("Unified_cpp_js_src18.o")
INPUT("Unified_cpp_js_src19.o")
INPUT("Unified_cpp_js_src2.o")
INPUT("Unified_cpp_js_src20.o")
INPUT("Unified_cpp_js_src21.o")
INPUT("Unified_cpp_js_src22.o")
INPUT("Unified_cpp_js_src23.o")
INPUT("Unified_cpp_js_src24.o")
INPUT("Unified_cpp_js_src25.o")
INPUT("Unified_cpp_js_src26.o")
INPUT("Unified_cpp_js_src27.o")
INPUT("Unified_cpp_js_src28.o")
INPUT("Unified_cpp_js_src29.o")
INPUT("Unified_cpp_js_src3.o")
INPUT("Unified_cpp_js_src30.o")
INPUT("Unified_cpp_js_src31.o")
INPUT("Unified_cpp_js_src32.o")
INPUT("Unified_cpp_js_src33.o")
INPUT("Unified_cpp_js_src34.o")
INPUT("Unified_cpp_js_src35.o")
INPUT("Unified_cpp_js_src36.o")
INPUT("Unified_cpp_js_src4.o")
INPUT("Unified_cpp_js_src5.o")
INPUT("Unified_cpp_js_src6.o")
INPUT("Unified_cpp_js_src7.o")
INPUT("Unified_cpp_js_src8.o")
INPUT("Unified_cpp_js_src9.o")
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so when searching for -lgcc_s
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so.1 when searching for libgcc_s.so.1
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so when searching for -lgcc_s
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so.1 when searching for libgcc_s.so.1
Unified_cpp_js_src0.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `GenerateJitExitStub(js::wasm::ModuleGenerator&, unsigned int, js::wasm::ProfilingOffsets*) [clone .constprop.369]':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `GenerateJitExitStub(js::wasm::ModuleGenerator&, unsigned int, js::wasm::ProfilingOffsets*) [clone .constprop.369]':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o:/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: more undefined references to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)' follow
Unified_cpp_js_src10.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src10.o: In function `js::jit::ICTableSwitch::Compiler::generateStubCode(js::jit::MacroAssembler&)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/BaselineIC.cpp:7656: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src10.o:/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: more undefined references to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)' follow
Unified_cpp_js_src11.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:540: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:540: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src11.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src11.o:/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: more undefined references to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)' follow
collect2: error: ld returned 1 exit status
make[3]: *** [libmozjs-47a1.so] Error 1
make[2]: *** [js/src/target] Error 2
make[1]: *** [compile] Error 2
make: *** [default] Error 2
Attachment #8718233 -
Flags: review?(r)
Assignee | ||
Comment 42•9 years ago
|
||
(In reply to Heiher [:hev] from comment #40)
> Yes. If one operand of branchTest32 is a real 64bits value, we need neuter
> the high bits of this value.
it needs neutering only with `cond == Zero || cond == NonZero`, right?
iiuc, `cond == Signed || cond == NotSigned` needs highest bit.
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Heiher [:hev] from comment #41)
> Comment on attachment 8718233 [details] [diff] [review]
> Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
>
> Review of attachment 8718233 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thank you fix, link failed:
Hmm, it works for me on osx.
Will try on linux.
Assignee | ||
Comment 44•9 years ago
|
||
The link error is not reproducible with arm64 simulator build, both on osx (clang-700.1.81) and linux (gcc (Debian 4.9.2-10) 4.9.2).
Tested on m-c ac39fba33c6d revision.
Today's m-c has another changes from bug 1240583, and it's not buildable with this patch :/
Added `Label*` instantiation for ma_b as a possible fix for following:
> undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
but I'm not sure how to fix other one, as there is already `wasm::JumpTarget` instantiation:
> undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
hev, can you take this over?
Attachment #8718233 -
Attachment is obsolete: true
Attachment #8718233 -
Flags: review?(branislav.rankov)
Flags: needinfo?(r)
Comment 45•9 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #44)
> Created attachment 8719138 [details] [diff] [review]
> Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
>
> The link error is not reproducible with arm64 simulator build, both on osx
> (clang-700.1.81) and linux (gcc (Debian 4.9.2-10) 4.9.2).
> Tested on m-c ac39fba33c6d revision.
> Today's m-c has another changes from bug 1240583, and it's not buildable
> with this patch :/
>
>
> Added `Label*` instantiation for ma_b as a possible fix for following:
> > undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
>
> but I'm not sure how to fix other one, as there is already
> `wasm::JumpTarget` instantiation:
> > undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
>
> hev, can you take this over?
OK. Please wait, I'm on holiday.
Flags: needinfo?(r)
Assignee | ||
Comment 46•9 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #44)
> Created attachment 8719138 [details] [diff] [review]
> Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
>
> The link error is not reproducible with arm64 simulator build
sorry, I meant, mips64 simulator build.
Comment 47•9 years ago
|
||
Comment on attachment 8718282 [details] [diff] [review]
Part 7: Move MacroAssembler::branchFloat into generic macro assembler.
Review of attachment 8718282 [details] [diff] [review]:
-----------------------------------------------------------------
Too bad you're not using MozReview for these code motions patches, it would make those trivial to review ;)
Thanks for doing this, though.
I'm pretty sure all platforms are doing the same things and a bit more refactoring could avoid platform specific code at all, but this is more work and outside the scope of this bug.
::: js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ +305,5 @@
> + j(Parity, &unordered);
> + j(Equal, label);
> + bind(&unordered);
> + return;
> + }
Can you add a new line after this }, please?
Attachment #8718282 -
Flags: review?(bbouvier) → review+
Comment 48•9 years ago
|
||
Comment on attachment 8719128 [details] [diff] [review]
Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler.
Review of attachment 8719128 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for the patch.
Attachment #8719128 -
Flags: review?(bbouvier) → review+
Comment 49•9 years ago
|
||
Comment on attachment 8719129 [details] [diff] [review]
Part 11: Move generic MacroAssembler methods into check_macroassembler_style block.
Review of attachment 8719129 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/MacroAssembler-inl.h
@@ +497,5 @@
> + MOZ_CRASH("Bad MIRType");
> + }
> +}
> +
> +template<typename T>
style-nit: extra space after template.
@@ +499,5 @@
> +}
> +
> +template<typename T>
> +void
> +MacroAssembler::branchKey(Condition cond, const T& length, const Int32Key& key, Label* label)
follow-up: Later, we should find a better name for Int32Key / branchKey. And replace these arguments names by lhs / rhs. It sounds to me that this function should just be named branch32.
::: js/src/jit/MacroAssembler.cpp
@@ +2507,5 @@
> +}
> +
> +void
> +MacroAssembler::branchEqualTypeIfNeeded(MIRType type, MDefinition* maybeDef, Register tag,
> + Label* label)
follow-up: What do you think about renaming this function maybeBranchTestType, or something similar?
Attachment #8719129 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 50•9 years ago
|
||
thank you for reviewing :)
I'll land those patches for now, and will prepare remaining parts.
for mips, will file a dedicated bug.
Keywords: leave-open
Assignee | ||
Comment 51•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f95c21f4dd507563eef8e861ed3fbc5051f7df49
Bug 1245112 - Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/e92294275e905741fe4a2c90e91a698d5a4d468f
Bug 1245112 - Part 2: Move MacroAssembler::branchPtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9f675232a0f3576de34a460991f27d27a67589
Bug 1245112 - Part 3: Move MacroAssembler::branch32 into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/db95ea81a22cf467b27e34a11d15e5849129ecf4
Bug 1245112 - Part 4: Move MacroAssembler::branchTest64 into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/e179ad3903cc1dc80fcac91743beb955301bcf41
Bug 1245112 - Part 5: Move MacroAssembler::branchTestPtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb37e43c52e762a24a0f6368c012e1a96fd0e75b
Bug 1245112 - Part 6: Move MacroAssembler::branchTest32 into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e0dc5c462b1663b89a7561a072ab4930184fb8
Bug 1245112 - Part 7: Move MacroAssembler::branchFloat into generic macro assembler. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e12bd365f3b236759a2e9a5c26780b13ddf15a7
Bug 1245112 - Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d78e70a6fcc02161fabaedb91b869c92f4e6cc
Bug 1245112 - Part 9: Move MacroAssembler::branchDouble into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffdbc9bf8d81eeac3509de568e85a614a396d83b
Bug 1245112 - Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c49177aabb67d2c6468823476057997954d5cd0
Bug 1245112 - Part 11: Move generic MacroAssembler methods into check_macroassembler_style block. r=nbp
Assignee | ||
Comment 52•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f884ed3994b4c7e866f2cefd93b639f1bb6d655
Bug 1245112 - Part 11 followup: Add a space after template. r=bustage
Assignee | ||
Comment 53•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/28f6cc5fa7c030346ba648a6d93d497f0b7334a7
Backed out changeset 7f884ed3994b (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d808e184fee32c9658718add02878c7ad016fc
Backed out changeset 6c49177aabb6 (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/be5c5019659ed6478b51b6d0906303c4fc2caa89
Backed out changeset ffdbc9bf8d81 (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5903ee04e8f4d09f83a1f9147bc7d8e088e669
Backed out changeset f8d78e70a6fc (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a9a917638aa330c87448fefe6987e74bd3ebc5
Backed out changeset 4e12bd365f3b (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d542d2a8eda1fc2d6359d1857651e701367b21ec
Backed out changeset 86e0dc5c462b (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/743285bf168063684a2f6bd18dba93bb0413ea42
Backed out changeset cb37e43c52e7 (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e95e4d405ef7b9927db7285bdac500e8dcb46ac
Backed out changeset e179ad3903cc (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e817588b35454ee21f6e2d3f95ce624b855f46
Backed out changeset db95ea81a22c (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/46bcabfc04c8fc1a2f6c17a12d0ffec54c678ca2
Backed out changeset 3d9f675232a0 (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/50279e2f620d40800a925d12322e824ed58db909
Backed out changeset e92294275e90 (bug 1245112)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3927e5cf68a2b9e9cb816aa30dc4e985a21c6b41
Backed out changeset f95c21f4dd50 (bug 1245112)
Assignee | ||
Comment 54•9 years ago
|
||
There is win64 specific issue with branchTest32 and Operand.
> c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/js/src/jit/x64/Trampoline-x64.cpp(97) : error C2664:
> 'void js::jit::MacroAssembler::branchTest32(js::jit::AssemblerX86Shared::Condition,const js::jit::AbsoluteAddress
> &,js::jit::Imm32,js::jit::Label *)' : cannot convert argument 2 from 'const js::jit::Operand' to 'js::jit::Register'
will try fixing it.
Assignee | ||
Comment 55•9 years ago
|
||
So I overlooked this single consumer of Operand variant of branchTest32.
it could just be replaced by Address.
Attachment #8719587 -
Flags: review?(nicolas.b.pierron)
Comment 56•9 years ago
|
||
Comment on attachment 8719587 [details] [diff] [review]
Part 6 followup
Review of attachment 8719587 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
Attachment #8719587 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 57•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae0d9254da420acea359d6c5160b07ae5b28f99
Bug 1245112 - Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/229cf45c17d5e08b6c7126f0e8fdd8feccad522d
Bug 1245112 - Part 2: Move MacroAssembler::branchPtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/44927d05b68566a70753227ba36e47051bc4092d
Bug 1245112 - Part 3: Move MacroAssembler::branch32 into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/279bdd1c241f16270add8d3cf7bc27159083dc12
Bug 1245112 - Part 4: Move MacroAssembler::branchTest64 into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/50fc6edd4906bfebdf4dc932a634a281850eadb6
Bug 1245112 - Part 5: Move MacroAssembler::branchTestPtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f433eabad0eecdaf0d895e68fae7f3501dd52c04
Bug 1245112 - Part 6: Move MacroAssembler::branchTest32 into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/38551974573c53bf0f4d47eb18c1dd56c1d3a734
Bug 1245112 - Part 7: Move MacroAssembler::branchFloat into generic macro assembler. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/97eff9ac5a850a73cd62cfc231aa3be3fc8e9d5b
Bug 1245112 - Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/88cc3e1cfb3b8e85166370bf3fac769e4bffe454
Bug 1245112 - Part 9: Move MacroAssembler::branchDouble into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb46f0c6477dc7b142a4e83633666d49b2784c9a
Bug 1245112 - Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4383349c6f4a57cd8ef8fc55de8cbdc215b63c
Bug 1245112 - Part 11: Move generic MacroAssembler methods into check_macroassembler_style block. r=nbp
Comment 58•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8ae0d9254da4
https://hg.mozilla.org/mozilla-central/rev/229cf45c17d5
https://hg.mozilla.org/mozilla-central/rev/44927d05b685
https://hg.mozilla.org/mozilla-central/rev/279bdd1c241f
https://hg.mozilla.org/mozilla-central/rev/50fc6edd4906
https://hg.mozilla.org/mozilla-central/rev/f433eabad0ee
https://hg.mozilla.org/mozilla-central/rev/38551974573c
https://hg.mozilla.org/mozilla-central/rev/97eff9ac5a85
https://hg.mozilla.org/mozilla-central/rev/88cc3e1cfb3b
https://hg.mozilla.org/mozilla-central/rev/fb46f0c6477d
https://hg.mozilla.org/mozilla-central/rev/9b4383349c6f
Assignee | ||
Comment 59•9 years ago
|
||
Attachment #8721743 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 60•9 years ago
|
||
Attachment #8721744 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 61•9 years ago
|
||
Attachment #8721745 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 62•9 years ago
|
||
Attachment #8721746 -
Flags: review?(lhansen)
Assignee | ||
Comment 63•9 years ago
|
||
Attachment #8721747 -
Flags: review?(lhansen)
Assignee | ||
Comment 64•9 years ago
|
||
Attachment #8721748 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 65•9 years ago
|
||
Attachment #8721749 -
Flags: review?(jdemooij)
Assignee | ||
Comment 66•9 years ago
|
||
Attachment #8721750 -
Flags: review?(jdemooij)
Comment 67•9 years ago
|
||
Comment on attachment 8721746 [details] [diff] [review]
Part 15: Move MacroAssembler::branchAdd32 into generic macro assembler.
Review of attachment 8721746 [details] [diff] [review]:
-----------------------------------------------------------------
Yes please.
Attachment #8721746 -
Flags: review?(lhansen) → review+
Updated•9 years ago
|
Attachment #8721747 -
Flags: review?(lhansen) → review+
Updated•9 years ago
|
Attachment #8721744 -
Flags: review?(jcoppeard) → review+
Updated•9 years ago
|
Attachment #8721745 -
Flags: review?(jcoppeard) → review+
Comment 68•9 years ago
|
||
Comment on attachment 8721749 [details] [diff] [review]
Part 18: Move MacroAssembler::branchTestInt32 into generic macro assembler.
Review of attachment 8721749 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +772,5 @@
>
> +void
> +MacroAssembler::branchTestInt32(Condition cond, Register tag, Label* label)
> +{
> + return branchTestInt32Impl(cond, tag, label);
Nit: it's a void function so you could remove the |return| here and below, same for some of the other files.
Attachment #8721749 -
Flags: review?(jdemooij) → review+
Comment 69•9 years ago
|
||
Comment on attachment 8721750 [details] [diff] [review]
Part 19: Move MacroAssembler::branchTestInt32Truthy into generic macro assembler.
Review of attachment 8721750 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks.
Attachment #8721750 -
Flags: review?(jdemooij) → review+
Comment 70•9 years ago
|
||
Comment on attachment 8721743 [details] [diff] [review]
Part 12: Move MacroAssembler::branchPtrWithPatch into generic macro assembler.
Review of attachment 8721743 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/x64/MacroAssembler-x64.h
@@ +545,5 @@
> return jumpWithPatch(label);
> }
>
> template <typename S, typename T>
> + CodeOffsetJump branchPtrWithPatchImpl(Condition cond, S lhs, T rhs, RepatchLabel* label) {
I do not see the benefit of an *Impl function for this tiny amount of code, removing it would have a diffstat of +4-7.
@@ +551,1 @@
> return jumpWithPatch(label, cond);
This implementation will work on both x86 and x64. Move it to x86-shared and use the PER_SHARED_ARCH macro.
Attachment #8721743 -
Flags: review?(nicolas.b.pierron) → review+
Updated•9 years ago
|
Attachment #8721748 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 72•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1a49cc5a331fadfd6fb43f391d0d108263038a
Bug 1245112 - Part 12: Move MacroAssembler::branchPtrWithPatch into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2411ecdbc3aa5d1bfc1997c8cdde5229ef37bc9
Bug 1245112 - Part 13: Move MacroAssembler::branchPtrInNurseryRange into generic macro assembler. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5db0c4768aec4597bb0a3dc06ab72a51bb29e2a4
Bug 1245112 - Part 14: Move MacroAssembler::branchValueIsNurseryObject into generic macro assembler. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/52aa480e7626544366090fc7bca0fd17f0168b19
Bug 1245112 - Part 15: Move MacroAssembler::branchAdd32 into generic macro assembler. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1c65dee0cb4fc921ce0cf05f7499c2a02f4b62
Bug 1245112 - Part 16: Move MacroAssembler::branchSub32 into generic macro assembler. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/df1a273493d59f3c9cf4d9278b6d799ddfb94f30
Bug 1245112 - Part 17: Move MacroAssembler::decBranchPtr into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/865173b1d24a5acb5e482c07584497a076f8cccc
Bug 1245112 - Part 18: Move MacroAssembler::branchTestInt32 into generic macro assembler. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/af10b351a596618049cab6c3d0acd131ea07d288
Bug 1245112 - Part 19: Move MacroAssembler::branchTestInt32Truthy into generic macro assembler. r=jandem
Comment 73•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b1a49cc5a33
https://hg.mozilla.org/mozilla-central/rev/c2411ecdbc3a
https://hg.mozilla.org/mozilla-central/rev/5db0c4768aec
https://hg.mozilla.org/mozilla-central/rev/52aa480e7626
https://hg.mozilla.org/mozilla-central/rev/2f1c65dee0cb
https://hg.mozilla.org/mozilla-central/rev/df1a273493d5
https://hg.mozilla.org/mozilla-central/rev/865173b1d24a
https://hg.mozilla.org/mozilla-central/rev/af10b351a596
Assignee | ||
Comment 74•9 years ago
|
||
Prepared remaining parts.
Even after applying all these patches, there are a lot of code duplications, such like a bunch of branchTest_TYPE_Impl() calls in branchTest_TYPE_() method, copied across x86-shared, arm, and arm64 archs.
The main issue is that mips32/mips64 doesn't have test_TYPE_() methods that returns Condition, as their branch instruction needs InstImm, not Condition. Otherwise we can merge most of implementations into single template method in MacroAssembler-inl.h, or MacroAssembler-nunbox32-inl.h/MacroAssembler-punbox64-inl.h mentioned in bug 996602.
So, I'd like to treat this bug as a preparation for further refactoring, and leave those dupliacations for now.
[Part 18 followup]
while moving other Value related methods, I noticed there are some more space for cleanup/refactoring in branchTestInt32, like aligning variable name with others, delegating branch code to Register variant, and removing redundant assertions.
In x64, only Address variant uses different code than original testInt32, that calls cmp32 directly,
https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/js/src/jit/x64/MacroAssembler-x64.h#727
> void branchTestInt32Impl(Condition cond, const Operand& operand, Label* label) {
> MOZ_ASSERT(cond == Equal || cond == NotEqual);
> cmp32(ToUpper32(operand), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_INT32))));
> j(cond, label);
> }
iiuc, it's better than testInt32 impl, as it does not require scratch register, right?
> Condition testInt32(Condition cond, const Address& src) {
> ScratchRegisterScope scratch(asMasm());
> splitTag(src, scratch);
> return testInt32(cond, scratch);
> }
changed testInt32 impl to use cmp32, and merged all x86 and x64 methods with testInt32+j.
Attachment #8723136 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 75•9 years ago
|
||
in remaining parts, they mostly follow the style as branchTestInt32, updated in Part 18 followup.
Attachment #8723141 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 76•9 years ago
|
||
Will ask review for other parts after getting review for parts 18.1, 20 and 21, as fix required there should also be applied to others.
Attachment #8723144 -
Flags: review?(nicolas.b.pierron)
Comment 77•9 years ago
|
||
Comment on attachment 8723136 [details] [diff] [review]
Part 18 followup: More refactor branchTestInt32.
Review of attachment 8723136 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/MacroAssembler.h
@@ +917,5 @@
> inline void branchKey(Condition cond, const T& length, const Int32Key& key, Label* label);
>
> inline void branchTestNeedsIncrementalBarrier(Condition cond, Label* label);
>
> + inline void branchTestInt32(Condition cond, Register tag, Label* label) PER_SHARED_ARCH;
nit: This variant of the function deserve a comment as it is different than the 4 others. The 4 other functions named branchTestInt32 are testing with either a Value or the address of a Value. As opposed to these function, this one check the register which contains the tag (32bits boxing), or the tagged value (64bits boxing).
@@ +926,2 @@
>
> + inline void branchTestInt32Truthy(bool truthy, const ValueOperand& value, Label* label)
nit: Add a comment above this function to describe how it is different than the previous one.
Attachment #8723136 -
Flags: review?(nicolas.b.pierron) → review+
Comment 78•9 years ago
|
||
Comment on attachment 8723141 [details] [diff] [review]
Part 20: Move MacroAssembler::branchTestDouble into generic macro assembler.
Review of attachment 8723141 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/MacroAssembler.h
@@ +926,5 @@
>
> inline void branchTestInt32Truthy(bool truthy, const ValueOperand& value, Label* label)
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>
> + inline void branchTestDouble(Condition cond, Register tag, Label* label)
Do the same as commented for branchTestInt32, and also for the following patches.
Attachment #8723141 -
Flags: review?(nicolas.b.pierron) → review+
Comment 79•9 years ago
|
||
Comment on attachment 8723144 [details] [diff] [review]
Part 21: Move MacroAssembler::branchTestDoubleTruthy into generic macro assembler.
Review of attachment 8723144 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/MacroAssembler.h
@@ +933,5 @@
> inline void branchTestDouble(Condition cond, const BaseIndex& address, Label* label) PER_SHARED_ARCH;
> inline void branchTestDouble(Condition cond, const ValueOperand& value, Label* label)
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>
> + inline void branchTestDoubleTruthy(bool truthy, FloatRegister reg, Label* label) PER_SHARED_ARCH;
nit: add a comment which describe what "Truthy" means.
Attachment #8723144 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 80•9 years ago
|
||
Added comments and private *Impl method.
can you review those parts?
will add each branchTest* methods to each comment sections, like following:
> // Perform a type-test on a tag of a Value (32bits boxing), or the tagged
> // value (64bits boxing).
> inline void branchTestInt32(Condition cond, Register tag, Label* label) PER_SHARED_ARCH;
> inline void branchTestDouble(Condition cond, Register tag, Label* label)
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>
> // Perform a type-test on a Value, addressed by Address or BaseIndex, or
> // loaded into ValueOperand.
> inline void branchTestInt32(Condition cond, const Address& address, Label* label) PER_SHARED_ARCH;
> inline void branchTestInt32(Condition cond, const BaseIndex& address, Label* label) PER_SHARED_ARCH;
> inline void branchTestInt32(Condition cond, const ValueOperand& value, Label* label)
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>
> inline void branchTestDouble(Condition cond, const Address& address, Label* label) PER_SHARED_ARCH;
> inline void branchTestDouble(Condition cond, const BaseIndex& address, Label* label) PER_SHARED_ARCH;
> inline void branchTestDouble(Condition cond, const ValueOperand& value, Label* label)
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>
> // Checks if given Value is evaluated to true or false in a condition.
> // Type of the value should match to the type of the method.
> inline void branchTestInt32Truthy(bool truthy, const ValueOperand& value, Label* label)
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
> inline void branchTestDoubleTruthy(bool truthy, FloatRegister reg, Label* label) PER_SHARED_ARCH;
>
> private:
>
> // Implementation for branchTest* methods.
> template <typename T>
> inline void branchTestInt32Impl(Condition cond, const T& t, Label* label)
> DEFINED_ON(arm, arm64, x86_shared);
> template <typename T>
> inline void branchTestDoubleImpl(Condition cond, const T& t, Label* label)
> DEFINED_ON(arm, arm64, x86_shared);
>
> //}}} check_macroassembler_style
Attachment #8723669 -
Flags: review?(nicolas.b.pierron)
Comment 81•9 years ago
|
||
Comment on attachment 8723669 [details] [diff] [review]
Part 18 followup: More refactor branchTestInt32.
Review of attachment 8723669 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/MacroAssembler.h
@@ +930,4 @@
> DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>
> + // Checks if given Value is evaluated to true or false in a condition.
> + // Type of the value should match to the type of the method.
nit: The type of the value should match the type of the method.
Attachment #8723669 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 82•9 years ago
|
||
here're remaining parts, that does almost same thing as Int32 and Double, for each type
Attachment #8724218 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 83•9 years ago
|
||
Attachment #8724225 -
Flags: review?(dvander)
Assignee | ||
Comment 84•9 years ago
|
||
Attachment #8724226 -
Flags: review?(dvander)
Assignee | ||
Comment 85•9 years ago
|
||
Attachment #8724227 -
Flags: review?(dvander)
Assignee | ||
Comment 86•9 years ago
|
||
Attachment #8724228 -
Flags: review?(dvander)
Assignee | ||
Comment 87•9 years ago
|
||
Attachment #8724229 -
Flags: review?(sstangl)
Assignee | ||
Comment 88•9 years ago
|
||
Attachment #8724231 -
Flags: review?(jorendorff)
Assignee | ||
Comment 89•9 years ago
|
||
Attachment #8724232 -
Flags: review?(sstangl)
Assignee | ||
Comment 90•9 years ago
|
||
Attachment #8724233 -
Flags: review?(sstangl)
Assignee | ||
Comment 91•9 years ago
|
||
Attachment #8724234 -
Flags: review?(jdemooij)
Assignee | ||
Comment 92•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8724235 -
Flags: review?(jdemooij)
Assignee | ||
Comment 93•9 years ago
|
||
Attachment #8724237 -
Flags: review?(jdemooij)
Assignee | ||
Comment 94•9 years ago
|
||
Attachment #8724238 -
Flags: review?(jdemooij)
Assignee | ||
Comment 95•9 years ago
|
||
Address variant of branchTestValue is not used, so removed.
Attachment #8724241 -
Flags: review?(jdemooij)
Assignee | ||
Comment 96•9 years ago
|
||
Part 2 added branchPtrImpl to x86/x64. moved them to genric MacroAssembler.
Attachment #8724381 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 97•9 years ago
|
||
fixed branchTestMagicImpl template for arm64.
Attachment #8724237 -
Attachment is obsolete: true
Attachment #8724237 -
Flags: review?(jdemooij)
Attachment #8724382 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Attachment #8724218 -
Flags: review?(bhackett1024) → review+
Attachment #8724225 -
Flags: review?(dvander) → review?(sstangl)
Attachment #8724226 -
Flags: review?(dvander) → review?(sstangl)
Attachment #8724227 -
Flags: review?(dvander) → review?(sstangl)
Attachment #8724228 -
Flags: review?(dvander) → review?(sstangl)
Updated•9 years ago
|
Attachment #8724381 -
Flags: review?(nicolas.b.pierron) → review+
Updated•9 years ago
|
Attachment #8724234 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8724235 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8724382 -
Flags: review?(jdemooij) → review+
Comment 98•9 years ago
|
||
Comment on attachment 8724238 [details] [diff] [review]
Part 34: Move MacroAssembler::branchTestMagicValue into generic macro assembler.
Review of attachment 8724238 [details] [diff] [review]:
-----------------------------------------------------------------
Excellent work, arai.
Attachment #8724238 -
Flags: review?(jdemooij) → review+
Comment 99•9 years ago
|
||
Comment on attachment 8724241 [details] [diff] [review]
Part 35: Move MacroAssembler::branchTestValue into generic macro assembler.
Review of attachment 8724241 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +386,5 @@
> j(truthy ? NonZero : Zero, label);
> }
>
> +void
> +MacroAssembler::branchTestValue(Condition cond, const ValueOperand& lhs,
Since the x86/arm/mips32 implementations are pretty long (the x86 version is at least 15 lines) I think it'd make sense to keep branchTestValue in the cpp files. What do you think?
Attachment #8724241 -
Flags: review?(jdemooij) → review+
Comment 100•9 years ago
|
||
Comment on attachment 8724225 [details] [diff] [review]
Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler.
Review of attachment 8724225 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +569,5 @@
> +
> +void
> +MacroAssembler::branchTestBoolean(Condition cond, const Address& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
Please don't hardcode SecondScratchReg: instead, use a ScratchRegisterScope.
@@ +576,5 @@
> +
> +void
> +MacroAssembler::branchTestBoolean(Condition cond, const BaseIndex& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
ScratchRegisterScope.
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +262,5 @@
>
> +void
> +MacroAssembler::branchTestBoolean(Condition cond, const ValueOperand& value, Label* label)
> +{
> + splitTag(value, SecondScratchReg);
Does MIPS64 have a ScratchRegsiterScope? MIPS32 does. If it does, please use that instead.
::: js/src/jit/x64/MacroAssembler-x64.h
@@ +332,5 @@
> return cond;
> }
> Condition testBoolean(Condition cond, const Address& src) {
> + cmp32(ToUpper32(src), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_BOOLEAN))));
> + return cond;
Nice.
Attachment #8724225 -
Flags: review?(sstangl) → review+
Comment 101•9 years ago
|
||
Comment on attachment 8724226 [details] [diff] [review]
Part 24: Move MacroAssembler::branchTestBooleanTruthy into generic macro assembler.
Review of attachment 8724226 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +269,5 @@
>
> +void
> +MacroAssembler::branchTestBooleanTruthy(bool b, const ValueOperand& value, Label* label)
> +{
> + unboxBoolean(value, SecondScratchReg);
Please use a ScratchRegisterScope. (I know the original code didn't... argh.)
Attachment #8724226 -
Flags: review?(sstangl) → review+
Comment 102•9 years ago
|
||
Comment on attachment 8724227 [details] [diff] [review]
Part 25: Move MacroAssembler::branchTestUndefined into generic macro assembler.
Review of attachment 8724227 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +518,5 @@
> +
> +void
> +MacroAssembler::branchTestUndefined(Condition cond, const Address& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
ScratchRegisterScope?
@@ +525,5 @@
> +
> +void
> +MacroAssembler::branchTestUndefined(Condition cond, const BaseIndex& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
ScratchRegisterScope?
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +227,5 @@
> void
> +MacroAssembler::branchTestUndefined(Condition cond, const ValueOperand& value,
> + Label* label)
> +{
> + splitTag(value, SecondScratchReg);
ScratchRegisterScope.
::: js/src/jit/x64/MacroAssembler-x64.h
@@ +323,5 @@
>
>
> Condition testUndefined(Condition cond, const Address& src) {
> + cmp32(ToUpper32(src), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_UNDEFINED))));
> + return cond;
Nice again.
Attachment #8724227 -
Flags: review?(sstangl) → review+
Comment 103•9 years ago
|
||
Ahem, those above should read "SecondScratchRegisterScope".
Comment 104•9 years ago
|
||
Comment on attachment 8724228 [details] [diff] [review]
Part 26: Move MacroAssembler::branchTestString into generic macro assembler.
Review of attachment 8724228 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +611,5 @@
> +
> +void
> +MacroAssembler::branchTestString(Condition cond, const BaseIndex& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
SecondScratchRegisterScope
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +284,5 @@
>
> +void
> +MacroAssembler::branchTestString(Condition cond, const ValueOperand& value, Label* label)
> +{
> + splitTag(value, SecondScratchReg);
SecondScratchRegisterScope
Attachment #8724228 -
Flags: review?(sstangl) → review+
Comment 105•9 years ago
|
||
Comment on attachment 8724229 [details] [diff] [review]
Part 27: Move MacroAssembler::branchTestStringTruthy into generic macro assembler.
Review of attachment 8724229 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips32/MacroAssembler-mips32-inl.h
@@ +349,5 @@
> +void
> +MacroAssembler::branchTestStringTruthy(bool b, const ValueOperand& value, Label* label)
> +{
> + Register string = value.payloadReg();
> + ma_lw(SecondScratchReg, Address(string, JSString::offsetOfLength()));
SecondScratchRegisterScope
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +291,5 @@
>
> +void
> +MacroAssembler::branchTestStringTruthy(bool b, const ValueOperand& value, Label* label)
> +{
> + unboxString(value, SecondScratchReg);
SecondScratchRegisterScope
Attachment #8724229 -
Flags: review?(sstangl) → review+
Comment 106•9 years ago
|
||
Comment on attachment 8724232 [details] [diff] [review]
Part 29: Move MacroAssembler::branchTestNull into generic macro assembler.
Review of attachment 8724232 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +639,5 @@
> +
> +void
> +MacroAssembler::branchTestNull(Condition cond, const Address& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
SecondScratchRegisterScope
@@ +646,5 @@
> +
> +void
> +MacroAssembler::branchTestNull(Condition cond, const BaseIndex& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
SecondScratchRegisterScope
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +306,5 @@
>
> +void
> +MacroAssembler::branchTestNull(Condition cond, const ValueOperand& value, Label* label)
> +{
> + splitTag(value, SecondScratchReg);
SecondScratchRegisterScope
Attachment #8724232 -
Flags: review?(sstangl) → review+
Comment 107•9 years ago
|
||
Comment on attachment 8724233 [details] [diff] [review]
Part 30: Move MacroAssembler::branchTestObject into generic macro assembler.
Review of attachment 8724233 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +660,5 @@
> +
> +void
> +MacroAssembler::branchTestObject(Condition cond, const Address& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
SecondScratchRegisterScope
@@ +667,5 @@
> +
> +void
> +MacroAssembler::branchTestObject(Condition cond, const BaseIndex& address, Label* label)
> +{
> + extractTag(address, SecondScratchReg);
SecondScratchRegisterScope
::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +313,5 @@
>
> +void
> +MacroAssembler::branchTestObject(Condition cond, const ValueOperand& value, Label* label)
> +{
> + splitTag(value, SecondScratchReg);
SecondScratchRegisterScope
Attachment #8724233 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 108•9 years ago
|
||
Thank you for reviewing :)
(In reply to Jan de Mooij [:jandem] from comment #99)
> Comment on attachment 8724241 [details] [diff] [review]
> Part 35: Move MacroAssembler::branchTestValue into generic macro assembler.
>
> Review of attachment 8724241 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: js/src/jit/x86/MacroAssembler-x86-inl.h
> @@ +386,5 @@
> > j(truthy ? NonZero : Zero, label);
> > }
> >
> > +void
> > +MacroAssembler::branchTestValue(Condition cond, const ValueOperand& lhs,
>
> Since the x86/arm/mips32 implementations are pretty long (the x86 version is
> at least 15 lines) I think it'd make sense to keep branchTestValue in the
> cpp files. What do you think?
I agree, moved to cpp locally.
(In reply to Sean Stangl [:sstangl] from comment #100)
> Comment on attachment 8724225 [details] [diff] [review]
> Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler.
>
> Review of attachment 8724225 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> @@ +569,5 @@
> > +
> > +void
> > +MacroAssembler::branchTestBoolean(Condition cond, const Address& address, Label* label)
> > +{
> > + extractTag(address, SecondScratchReg);
>
> Please don't hardcode SecondScratchReg: instead, use a ScratchRegisterScope.
Applied to all other not-yet-landed patches too.
for already-landed patches, will post followup.
Updated•9 years ago
|
Attachment #8724231 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 109•9 years ago
|
||
thank you!
there is already bug 1205524 for ScratchRegisterScope on mips.
will land remaining patches shortly.
Keywords: leave-open
Assignee | ||
Comment 110•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/65554e19955c8a610addf7d05e7fa889e53246ca
Bug 1245112 - Part 18 followup: More refactor branchTestInt32. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bfd44ff31694b1c72c1bf87ce65098310af710
Bug 1245112 - Part 20: Move MacroAssembler::branchTestDouble into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/63f26d3fe989ec5ed10950328d68c957bac9990d
Bug 1245112 - Part 21: Move MacroAssembler::branchTestDoubleTruthy into generic macro assembler. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/6db0c9d5144e0fc9cd98cd33a440ac970ab008f0
Bug 1245112 - Part 22: Move MacroAssembler::branchTestNumber into generic macro assembler. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28770d913990481aa6e5054924d6f6a2023b470
Bug 1245112 - Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/53abfbd00384ab2656296de21621ff093aab90db
Bug 1245112 - Part 24: Move MacroAssembler::branchTestBooleanTruthy into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7cc18bce598c3f843c0ed5e7ad582042629971d
Bug 1245112 - Part 25: Move MacroAssembler::branchTestUndefined into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/0915a28459b5e4550ffe3b9e62775c47435f19c7
Bug 1245112 - Part 26: Move MacroAssembler::branchTestString into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/067c628f3135648e001eb10bf34fe1de505d1511
Bug 1245112 - Part 27: Move MacroAssembler::branchTestStringTruthy into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a49e013b77aa86ae8711b4d69362141fe6e54f
Bug 1245112 - Part 28: Move MacroAssembler::branchTestSymbol into generic macro assembler. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e20d12317e31fa07fd6f1490da6c2b92baa5ca4
Bug 1245112 - Part 29: Move MacroAssembler::branchTestNull into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bac6341260b00cbac90fa49390ebec01a50b662
Bug 1245112 - Part 30: Move MacroAssembler::branchTestObject into generic macro assembler. r=sstangl
https://hg.mozilla.org/integration/mozilla-inbound/rev/d95aba27394f2d07475b1ed2b8eb12083c1c23d9
Bug 1245112 - Part 31: Move MacroAssembler::branchTestGCThing into generic macro assembler. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/444cfe1bcbc76fc29d7c6adf85f7225a2f37a837
Bug 1245112 - Part 32: Move MacroAssembler::branchTestPrimitive into generic macro assembler. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab4ba99b59e865c98a17bede6f3085c2fc5592b0
Bug 1245112 - Part 33: Move MacroAssembler::branchTestMagic into generic macro assembler. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dee595021cd49d155cd7e123fd7741a3657c021
Bug 1245112 - Part 34: Move MacroAssembler::branchTestMagicValue into generic macro assembler. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a2e1207697113d12dda88a959a7d1768309dc4
Bug 1245112 - Part 35: Move MacroAssembler::branchTestValue into generic macro assembler. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7889f529f6c83d3c90f83ab053cf062057ddbeed
Bug 1245112 - Part 36: Move MacroAssembler::branchPtrImpl into generic macro assembler. r=nbp
Comment 111•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/65554e19955c
https://hg.mozilla.org/mozilla-central/rev/e8bfd44ff316
https://hg.mozilla.org/mozilla-central/rev/63f26d3fe989
https://hg.mozilla.org/mozilla-central/rev/6db0c9d5144e
https://hg.mozilla.org/mozilla-central/rev/e28770d91399
https://hg.mozilla.org/mozilla-central/rev/53abfbd00384
https://hg.mozilla.org/mozilla-central/rev/d7cc18bce598
https://hg.mozilla.org/mozilla-central/rev/0915a28459b5
https://hg.mozilla.org/mozilla-central/rev/067c628f3135
https://hg.mozilla.org/mozilla-central/rev/18a49e013b77
https://hg.mozilla.org/mozilla-central/rev/4e20d12317e3
https://hg.mozilla.org/mozilla-central/rev/6bac6341260b
https://hg.mozilla.org/mozilla-central/rev/d95aba27394f
https://hg.mozilla.org/mozilla-central/rev/444cfe1bcbc7
https://hg.mozilla.org/mozilla-central/rev/ab4ba99b59e8
https://hg.mozilla.org/mozilla-central/rev/3dee595021cd
https://hg.mozilla.org/mozilla-central/rev/81a2e1207697
https://hg.mozilla.org/mozilla-central/rev/7889f529f6c8
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•