1301400 - Enable the Wasm baseline compiler for x86 again (FF52)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

Same as bug 1290453, but the remaining part. Opening a new bug, since these patches will most likely land after the merge. Therefore it is better to have a new bug for tracking.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Basic changes to support int64 ops in x86

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part 2: WasmTruncateI64

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part 3: ConvertI64

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part 4: QuotientI64 & RemainderI64

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Part 5: LoadI64 & StoreI64

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Part 6: PopI64

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Part 7: PassArgI64

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Part 8: SubtractI64

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part 9: MultiplyI64

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Part 10: BitOpI64

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Part 11: ShiftI64

Comment 12

[Hannes Verschore [:h4writer]]

Attachment: Part 12: RotateI64

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: Part 13: ClzI64 & CtzI64

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: Part 14: PopcntI64

Comment 15

[Hannes Verschore [:h4writer]]

Attachment: Part 15: ReinterpretI64

Comment 16

[Hannes Verschore [:h4writer]]

Attachment: Part 16: ExtendI64 & WrapI64

Comment 17

[Hannes Verschore [:h4writer]]

Attachment: Part 17: Cmp64Set

Comment 18

[Hannes Verschore [:h4writer]]

Attachment: Part 18: LoadGlobalVarI64 & StoreGlobalVarI64

Comment 19

[Hannes Verschore [:h4writer]]

Attachment: Part 19: Enable the wasm compiler for x86 again

Comment 20

[Lars T Hansen [:lth]]

Comment on attachment 8789383 [details] [diff] [review]
Part 2: WasmTruncateI64

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

::: js/src/jit/x64/MacroAssembler-x64.cpp
@@ +188,2 @@
>      j(Assembler::Signed, oolEntry);
> +    asMasm().or64(Imm64(0x8000000000000000), Register64(output.reg));

Just use "output" here?

@@ +216,2 @@
>      j(Assembler::Signed, oolEntry);
> +    asMasm().or64(Imm64(0x8000000000000000), Register64(output.reg));

Ditto?

Comment 21

[Lars T Hansen [:lth]]

Comment on attachment 8789384 [details] [diff] [review]
Part 3: ConvertI64

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

OK with indicated fix, plus note comments about freeI32() and how that path might have to change.

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +4549,1 @@
>      freeI64(r0);

Need to free the temp here (under a guard maybe, see next).

@@ +4602,2 @@
>      freeI64(r0);
> +    freeI32(temp);

Really would prefer if freeI32() did not allow an invalid register; in that case there would need to be a guard here.  (The patch that adds that path to freeI32 hasn't been approved yet.)

Comment 22

[Lars T Hansen [:lth]]

Comment on attachment 8789385 [details] [diff] [review]
Part 4: QuotientI64 & RemainderI64

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

Some nits I'd like to see fixed, but otherwise looks nice.

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +493,5 @@
>  #if defined(JS_CODEGEN_X86)
>      AllocatableGeneralRegisterSet singleByteRegs_;
>  #endif
>  
> +    RegI64 abiReturnRegI64;

This wants an ifdef, I think?  Only used for NUNBOX_32.

@@ +2357,5 @@
>          }
>          masm.bind(&notMin);
>      }
>  
> +    void checkDivideSignedOverflowI64(RegI64 rhs, RegI64 lhsDest, Label* done, bool zeroOnOverflow) {

srcDest is used everywhere else, and lhsDest is not used anywhere else before this; would prefer the current state of affairs didn't change.  (I'm not claiming "lhs" is not used by itself, but it's used very few places.)

@@ +3700,2 @@
>      RegI64 r0, r1;
> + #ifdef JS_CODEGEN_X64

I don't care much, but the style in the rest of the file is "# ifdef" instead of " #ifdef".

@@ +5575,5 @@
> +    Label done;
> +
> +    sync();
> +
> +    MOZ_ASSERT(output.reg == lhs.reg);

Why not use a srcDest pattern for the signature here?

@@ +6899,5 @@
>        specific_edx(RegI32(edx)),
>  #endif
>  #ifdef JS_CODEGEN_X86
>        singleByteRegs_(GeneralRegisterSet(Registers::SingleByteRegs)),
> +      abiReturnRegI64(RegI64(Register64(edx, eax))),

Do we need something for ARM, so that this initialization isn't missed when ARM lands, or will the register being invalid cause assertions quickly enough?

Comment 23

[Lars T Hansen [:lth]]

Comment on attachment 8789388 [details] [diff] [review]
Part 7: PassArgI64

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

A couple of things that appear to need attention, otherwise this seems like a fine solution.  (Kind of gross with the code (almost-)duplication but there's no obvious fix to that.)

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +1071,5 @@
>              MOZ_CRASH("Compiler bug: Expected int on stack");
>          }
>      }
>  
> +    void loadI64Low(Register r, Stk& src) {

An ifdef JS_NUNBOX32 around these two functions?  I'm surprised that they even compile on a 64-bit system, given that they access the .low and .high parts of Register64.

@@ +1105,5 @@
> +          case Stk::LocalI64:
> +            loadFromFrameI32(r, frameOffsetFromSlot(src.slot(), MIRType::Int64) - INT64HIGH_OFFSET);
> +            break;
> +          case Stk::RegisterI64:
> +            if (src.i64reg().reg.low != r)

Surely ".high"?

Comment 24

[Lars T Hansen [:lth]]

Comment on attachment 8789391 [details] [diff] [review]
Part 8: SubtractI64

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

Sweet.

Comment 25

[Lars T Hansen [:lth]]

Comment on attachment 8789392 [details] [diff] [review]
Part 9: MultiplyI64

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

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +3636,2 @@
>      freeI64(r1);
> +    freeI32(temp);

freeI32() probably needs a guard, see earlier discussion.

Comment 26

[Lars T Hansen [:lth]]

Comment on attachment 8789404 [details] [diff] [review]
Part 17: Cmp64Set

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

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +2811,5 @@
>  #if defined(JS_CODEGEN_X64)
>          masm.cmpq(rhs.reg.reg, lhs.reg.reg);
>          masm.emitSet(cond, dest.reg);
> +#elif defined(JS_CODEGEN_X86)
> +        Label done, equal;

"equal" is a misleading label name here, how about "condTrue" or something like that?

Comment 27

[Lars T Hansen [:lth]]

Comment on attachment 8789386 [details] [diff] [review]
Part 5: LoadI64 & StoreI64

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

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +853,5 @@
> +        vmovsdWithPatch(value.fpu(), dstAddr);
> +        break;
> +
> +      case Scalar::Int64:
> +        MOZ_CRASH("Should be handled in storeI64.");

"storeI64"?

Comment 28

[Lars T Hansen [:lth]]

Comment on attachment 8789382 [details] [diff] [review]
Part 1: Basic changes to support int64 ops in x86

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

Primarily, I think we need to come up with a different solution to the problem you're trying to solve with the change to fromI32().  There are three subsequent patches that depend on this, I'll leave them unreviewed for the time being.

Secondarily the functionality added to freeGPR really belongs in the callers.

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +644,5 @@
>      }
>  
>      void freeGPR(Register r) {
> +        if (r == Register::Invalid())
> +            return;

I don't think this is a good idea.  There are a couple of subsequent patches that depend on this, but I would like to see the guard lifted out into the callers.  The invariant here should be that on entry, r is allocated and on exit it is not.  The convenience of having the guard here seems very small.

@@ +893,5 @@
> +        RegI32 high = needI32();
> +        if (high == r) {
> +            high = needI32();
> +            freeI32(r);
> +        }

This strikes me as the wrong thing, as previously discussed on IRC.  That has implications for three subsequent patches (from what I can see).

I think it should be an invariant here that, on entry, r is live (allocated).  On exit, r is the low part of the returned register and is therefore still live.

::: js/src/jit/x86-shared/Assembler-x86-shared.h
@@ +166,5 @@
>          switch (kind()) {
>            case REG:          return r.encoding() == reg();
>            case MEM_REG_DISP: return r.encoding() == base();
>            case MEM_SCALE:    return r.encoding() == base() || r.encoding() == index();
> +          default:           return false;

Nice catch.  I would probably prefer an exhaustive list of cases here, but that's just my opinion.

Comment 29

[Hannes Verschore [:h4writer]]

Attachment: Interdiff

Based on top of all the above patches, this adjusts fromI32 and fromI64 like requested together with the changes to clz/shift/... This should fix the issues raised in Part 1. I posted the diff, since I think that would be easier? Feel free to ask me to put these changes in the actual patches. But since the changes aren't that huge, I thought this was easier for the reviewer?

Comment 30

[Lars T Hansen [:lth]]

Comment on attachment 8793694 [details] [diff] [review]
Interdiff

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

Consider this r+ for the remaining patches.

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +905,4 @@
>          freeInt64(r.reg);
>      }
>  
> +    void freeI64Except(RegI64 r, RegI32 except) {

OK.  I think there's a preexisting bug in the compiler where fromI64() is called just that but does not free the high part (because it can't).  I think I should look into that and rename both that one (eg to "overlayI32") and this one (to "fromI64") so that fromI32 and fromI64 are dual in their allocation/deallocation behavior.  But you don't have to be burdened with that, if you want to just land this one as it is.

@@ +4164,5 @@
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
> +    needI32(specific_ecx);
> +    r1 = fromI32(specific_ecx);
> +    freeI64(r1);
> +    r1 = popI64(r1);

Use popI64ToSpecific() instead of free + pop, since the former is a well-defined abstraction for this.

@@ +4205,5 @@
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
> +    needI32(specific_ecx);
> +    r1 = fromI32(specific_ecx);
> +    freeI64(r1);
> +    r1 = popI64(r1);

Same.

@@ +4246,5 @@
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
> +    needI32(specific_ecx);
> +    r1 = fromI32(specific_ecx);
> +    freeI64(r1);
> +    r1 = popI64(r1);

Same.

@@ +4281,5 @@
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
> +    needI32(specific_ecx);
> +    r1 = fromI32(specific_ecx);
> +    freeI64(r1);
> +    r1 = popI64(r1);

Same.

@@ +4316,5 @@
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
> +    needI32(specific_ecx);
> +    r1 = fromI32(specific_ecx);
> +    freeI64(r1);
> +    r1 = popI64(r1);

Same.

Comment 31

[Lars T Hansen [:lth]]

Comment on attachment 8789395 [details] [diff] [review]
Part 11: ShiftI64

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

r+ with proposed changes.

Comment 32

[Lars T Hansen [:lth]]

Comment on attachment 8789396 [details] [diff] [review]
Part 12: RotateI64

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

r+ with proposed changes.

Comment 33

[Lars T Hansen [:lth]]

Comment on attachment 8789403 [details] [diff] [review]
Part 16: ExtendI64 & WrapI64

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

r+ with proposed changes.

Comment 34

[Hannes Verschore [:h4writer]]

Attachment: Part 20: Disable architecture if used functionality is not yet ported to that specific architecture.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58829d3beda3
Baseline Wasm Compiler: Part 1: Basic changes to support int64 ops in x86, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b4fdd7588a
Baseline Wasm Compiler: Part 2: Implement WasmTruncateI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c97d5898871
Baseline Wasm Compiler: Part 3: Implement ConvertI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/90989a3b070d
Baseline Wasm Compiler: Part 4: Implement QuotientI64 and RemainderI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6daefd4e79
Baseline Wasm Compiler: Part 5: Implement LoadI64 and StoreI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a878585ab510
Baseline Wasm Compiler: Part 6: Implement PopI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a11c6866c92
Baseline Wasm Compiler: Part 7: Implement PassArgI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/72b020fd3ce1
Baseline Wasm Compiler: Part 8: Implement SubtractI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05925c3a813
Baseline Wasm Compiler: Part 9: Implement MultiplyI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/64597bec5f4e
Baseline Wasm Compiler: Part 10: Implement BitopI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4decf2703b2
Baseline Wasm Compiler: Part 11: Implement ShiftI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bbefafb7c4
Baseline Wasm Compiler: Part 12: Implement RotateI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2347550ce36
Baseline Wasm Compiler: Part 13: Implement ClzI64 and CtzI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3245fe9958e
Baseline Wasm Compiler: Part 14: Implement PopcntI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1acadee786e9
Baseline Wasm Compiler: Part 15: Implement ReinterpretI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef547be7cb2f
Baseline Wasm Compiler: Part 16: Implement ExtendI64 and WrapI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/78cc93919411
Baseline Wasm Compiler: Part 17: Implement Cmp64Set, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ba0c152db6
Baseline Wasm Compiler: Part 18: Implement LoadGlobalVarI64 and StoreGlobalVarI64, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4dbc8dc886
Baseline Wasm Compiler: Part 19: Enable the wasm compiler for x86 again, r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9643bfae6b31
Baseline Wasm Compiler: Part 20: Don't fully generalize when functions are not yet available on every platform, r=lth

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58829d3beda3
https://hg.mozilla.org/mozilla-central/rev/07b4fdd7588a
https://hg.mozilla.org/mozilla-central/rev/7c97d5898871
https://hg.mozilla.org/mozilla-central/rev/90989a3b070d
https://hg.mozilla.org/mozilla-central/rev/7d6daefd4e79
https://hg.mozilla.org/mozilla-central/rev/a878585ab510
https://hg.mozilla.org/mozilla-central/rev/3a11c6866c92
https://hg.mozilla.org/mozilla-central/rev/72b020fd3ce1
https://hg.mozilla.org/mozilla-central/rev/c05925c3a813
https://hg.mozilla.org/mozilla-central/rev/64597bec5f4e
https://hg.mozilla.org/mozilla-central/rev/e4decf2703b2
https://hg.mozilla.org/mozilla-central/rev/f5bbefafb7c4
https://hg.mozilla.org/mozilla-central/rev/f2347550ce36
https://hg.mozilla.org/mozilla-central/rev/e3245fe9958e
https://hg.mozilla.org/mozilla-central/rev/1acadee786e9
https://hg.mozilla.org/mozilla-central/rev/ef547be7cb2f
https://hg.mozilla.org/mozilla-central/rev/78cc93919411
https://hg.mozilla.org/mozilla-central/rev/07ba0c152db6
https://hg.mozilla.org/mozilla-central/rev/ef4dbc8dc886
https://hg.mozilla.org/mozilla-central/rev/9643bfae6b31