1303690 - Baldr: MIPS: Take alignment hints into account when compiling load/store

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[hev [:hev]]

See Bug 1283126

Comment 1

[hev [:hev]]

Attachment: 0001-Bug-1303690-Baldr-MIPS-Take-alignment-hints-into-acc.patch

Comment 2

[hev [:hev]]

Attachment: 0001-Bug-1303690-Baldr-MIPS-Take-alignment-hints-into-acc.patch

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8794074 [details] [diff] [review]
0001-Bug-1303690-Baldr-MIPS-Take-alignment-hints-into-acc.patch

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

Looks great, thanks. Sorry for the slow review.

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +1934,5 @@
> +                masm.loadUnalignedFloat32(BaseIndex(HeapReg, ptr, TimesOne), temp,
> +                                          ToFloatRegister(lir->output()));
> +            } else
> +                masm.loadUnalignedDouble(BaseIndex(HeapReg, ptr, TimesOne), temp,
> +                                         ToFloatRegister(lir->output()));

nit: can you hoist BaseIndex(...) above the if, and ToFloatRegister(lir->output()) too? This way, I think the two calls to masm.loadUnaligned* will fit on one line and you can remove the braces currently necessary for the `if` (and currently missing for the `else`)

@@ +2018,5 @@
> +                masm.storeUnalignedFloat32(ToFloatRegister(lir->value()), temp,
> +                                           BaseIndex(HeapReg, ptr, TimesOne));
> +            } else
> +                masm.storeUnalignedDouble(ToFloatRegister(lir->value()), temp,
> +                                          BaseIndex(HeapReg, ptr, TimesOne));

same remarks here

::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
@@ +338,5 @@
> +        if (ins->offset())
> +            lir->setTemp(0, tempCopy(base, 0));
> +
> +        define(lir, ins);
> +

nit: extra line here

@@ +384,5 @@
> +        if (ins->offset())
> +            lir->setTemp(0, tempCopy(base, 0));
> +
> +        add(lir, ins);
> +

nit: extra line here

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +446,5 @@
>  }
>  
>  void
> +MacroAssemblerMIPSShared::ma_load_unaligned(Register dest, const BaseIndex& src,
> +                                  Register temp, LoadStoreSize size, LoadStoreExtension extension)

nit: align `Register temp` with `Register dest`

@@ +464,5 @@
> +        encodedOffset = Imm16(0).encode();
> +    }
> +
> +    switch (size) {
> +      case SizeByte:

Can a 1-byte load be misaligned on MIPS? Otherwise, I think you could just MOZ_ASSERT(size != SizeByte) somewhere in this function.

@@ +465,5 @@
> +    }
> +
> +    switch (size) {
> +      case SizeByte:
> +        if (ZeroExtend == extension)

nit: can you invert LHS and RHS? It's usual to have the constant part on the right hand side.

@@ +472,5 @@
> +            as_lb(dest, base, encodedOffset);
> +        break;
> +      case SizeHalfWord:
> +        as_lbu(dest, base, encodedOffset);
> +        as_lbu(temp, base, encodedOffset + 1);

It's weird to manipulate encodedOffset here, because we implicitly rely on the fact that

encodedOffset + 1 = Imm16(src.offset + 1).encode();

Can you either add a comment or reconstruct the Imm16 from encodedOffset, add 1 and call encode() again?

@@ +474,5 @@
> +      case SizeHalfWord:
> +        as_lbu(dest, base, encodedOffset);
> +        as_lbu(temp, base, encodedOffset + 1);
> +        as_ins(dest, temp, 8, 8);
> +        if (ZeroExtend != extension)

nit: can you invert RHS and LHS here too

@@ +475,5 @@
> +        as_lbu(dest, base, encodedOffset);
> +        as_lbu(temp, base, encodedOffset + 1);
> +        as_ins(dest, temp, 8, 8);
> +        if (ZeroExtend != extension)
> +            as_seh(dest, dest);

Instead of this with the seh, could you do the following:

as_lbu(dest, base, encodedOffset);
if (extension == ZeroExtend)
    as_lbu(temp, base, encodedOffset + 1);
else
    as_lb(temp, base, encodedOffset + 1);
as_ins(dest, temp, 8, 24);

(or something resembling this)

Unless of course your initial sequence is faster in number of cycles etc.

@@ +481,5 @@
> +      case SizeWord:
> +        as_lwl(dest, base, encodedOffset + 3);
> +        as_lwr(dest, base, encodedOffset);
> +        if (ZeroExtend == extension)
> +            as_ext(dest, dest, 0, 32);

Would there be a similar strategy to avoid the sign extension? (i.e. is there such an instruction like as_lwlu?)

@@ +594,5 @@
>  }
>  
> +void
> +MacroAssemblerMIPSShared::ma_store_unaligned(Register data, const BaseIndex& dest,
> +                                   Register temp, LoadStoreSize size, LoadStoreExtension extension)

nit: align both `Register`, please

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.h
@@ +105,5 @@
>      void ma_load(Register dest, const BaseIndex& src, LoadStoreSize size = SizeWord,
>                   LoadStoreExtension extension = SignExtend);
> +    void ma_load_unaligned(Register dest, const BaseIndex& src, Register temp,
> +                           LoadStoreSize size = SizeWord,
> +                           LoadStoreExtension extension = SignExtend);

I'd really prefer that all the parameters be not-optional, because the choice of default values here seem dubious to me.

@@ +114,5 @@
>      void ma_store(Imm32 imm, const BaseIndex& dest, LoadStoreSize size = SizeWord,
>                    LoadStoreExtension extension = SignExtend);
> +    void ma_store_unaligned(Register data, const BaseIndex& dest, Register temp,
> +                            LoadStoreSize size = SizeWord,
> +                            LoadStoreExtension extension = SignExtend);

ditto

::: js/src/jit/mips32/CodeGenerator-mips32.cpp
@@ +502,5 @@
> +            if (!isSigned)
> +                masm.move32(Imm32(0), output.high);
> +            else
> +                masm.ma_sra(output.high, output.low, Imm32(31));
> +

nit: extra blank line

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +1203,5 @@
> +        as_lwl(temp, ScratchRegister, 3);
> +        as_lwr(temp, ScratchRegister, 0);
> +    }
> +
> +    moveToFloat32(temp, dest);

Looks like this method is the same on mips32 (unless i missed something), can you hoist it in MacroAssemblerMIPSShared?

@@ +1354,5 @@
> +        ma_li(ScratchRegister, Imm32(dest.offset));
> +        as_daddu(ScratchRegister, SecondScratchReg, ScratchRegister);
> +        as_swl(temp, ScratchRegister, 3);
> +        as_swr(temp, ScratchRegister, 0);
> +    }

ditto

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25bc09ae4be0
Baldr: MIPS: Take alignment hints into account when compiling load/store. r=bbouvier

Comment 5

[hev [:hev]]

Attachment: 0001-Bug-1303690-Baldr-MIPS-Fix-alignment-hints-after-rev.patch

Sorry for missed to fold revision.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8799400 [details] [diff] [review]
0001-Bug-1303690-Baldr-MIPS-Fix-alignment-hints-after-rev.patch

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

No worries, thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85239a8b7217
Baldr: MIPS: Fix alignment hints after review. r=bbouvier

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25bc09ae4be0
https://hg.mozilla.org/mozilla-central/rev/85239a8b7217