Enable the Wasm baseline compiler for x86 again (FF52)

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(21 attachments)

9.67 KB, patch
lth
: review-
Details | Diff | Splinter Review
21.65 KB, patch
lth
: review+
Details | Diff | Splinter Review
29.41 KB, patch
lth
: review+
Details | Diff | Splinter Review
12.23 KB, patch
lth
: review+
Details | Diff | Splinter Review
34.56 KB, patch
lth
: review+
Details | Diff | Splinter Review
659 bytes, patch
lth
: review+
Details | Diff | Splinter Review
3.62 KB, patch
lth
: review+
Details | Diff | Splinter Review
995 bytes, patch
lth
: review+
Details | Diff | Splinter Review
2.17 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.42 KB, patch
lth
: review+
Details | Diff | Splinter Review
3.49 KB, patch
lth
: review+
Details | Diff | Splinter Review
2.76 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.33 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.81 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.40 KB, patch
lth
: review+
Details | Diff | Splinter Review
2.50 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.00 KB, patch
lth
: review+
Details | Diff | Splinter Review
2.16 KB, patch
lth
: review+
Details | Diff | Splinter Review
807 bytes, patch
lth
: review+
Details | Diff | Splinter Review
4.20 KB, patch
lth
: review+
Details | Diff | Splinter Review
9.91 KB, patch
lth
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
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.
Assignee

Updated

3 years ago
Depends on: 1290453
Assignee

Comment 1

3 years ago
Assignee: nobody → hv1989
Assignee

Comment 6

3 years ago
Assignee

Updated

3 years ago
Attachment #8789382 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789383 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789384 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789385 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789386 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789387 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789388 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789391 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789392 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789394 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789395 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789396 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789398 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789399 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789400 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789403 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789404 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789405 - Flags: review?(lhansen)
Assignee

Updated

3 years ago
Attachment #8789406 - Flags: review?(lhansen)
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?
Attachment #8789383 - Flags: review?(lhansen) → review+
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.)
Attachment #8789384 - Flags: review?(lhansen) → review+
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?
Attachment #8789385 - Flags: review?(lhansen) → review+
Attachment #8789387 - Flags: review?(lhansen) → review+
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"?
Attachment #8789388 - Flags: review?(lhansen) → review+
Comment on attachment 8789391 [details] [diff] [review]
Part 8: SubtractI64

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

Sweet.
Attachment #8789391 - Flags: review?(lhansen) → review+
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.
Attachment #8789392 - Flags: review?(lhansen) → review+
Attachment #8789394 - Flags: review?(lhansen) → review+
Attachment #8789398 - Flags: review?(lhansen) → review+
Attachment #8789399 - Flags: review?(lhansen) → review+
Attachment #8789400 - Flags: review?(lhansen) → review+
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?
Attachment #8789404 - Flags: review?(lhansen) → review+
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"?
Attachment #8789386 - Flags: review?(lhansen) → review+
Attachment #8789406 - Flags: review?(lhansen) → review+
Attachment #8789405 - Flags: review?(lhansen) → review+
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.
Attachment #8789382 - Flags: review?(lhansen) → review-
Assignee

Comment 29

3 years ago
Posted patch InterdiffSplinter Review
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?
Attachment #8793694 - Flags: review?(lhansen)
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.
Attachment #8793694 - Flags: review?(lhansen) → review+
Comment on attachment 8789395 [details] [diff] [review]
Part 11: ShiftI64

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

r+ with proposed changes.
Attachment #8789395 - Flags: review?(lhansen) → review+
Comment on attachment 8789396 [details] [diff] [review]
Part 12: RotateI64

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

r+ with proposed changes.
Attachment #8789396 - Flags: review?(lhansen) → review+
Comment on attachment 8789403 [details] [diff] [review]
Part 16: ExtendI64 & WrapI64

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

r+ with proposed changes.
Attachment #8789403 - Flags: review?(lhansen) → review+
Attachment #8796216 - Flags: review?(lhansen) → review+

Comment 35

3 years ago
Pushed by hv1989@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.