Closed Bug 1420104 Opened 7 years ago Closed 7 years ago

Wasm baseline: Vacuum, dust, and mop

Categories

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

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(5 files)

Bug 1420104 - rabaldr, reorganize and tidy up.
59 bytes, text/x-review-board-request
bbouvier
: review+
Details
Bug 1420104 - rabaldr, fix operand order when loading from value stack.
59 bytes, text/x-review-board-request
bbouvier
: review+
Details
Bug 1420104 - rabaldr, fix operand order when loading from CPU stack and locals.
59 bytes, text/x-review-board-request
bbouvier
: review+
Details
Bug 1420104 - rabaldr, clean up temp register managements and some ifdefs.
59 bytes, text/x-review-board-request
bbouvier
: review+
Details
Bug 1420104 - rabaldr, remove more ifdefs, simplify more.
59 bytes, text/x-review-board-request
bbouvier
: review+
Details 
Basically these cleanup items:

- textually separate operations on the value stack (Stk&) from other 
  operations
- move a few other operations around so that they are in the appropriate
  textual sections
- clean up the use of invalid registers (left over from bug 1419025)
- consistently use abstractions for loading constants and reg-reg moves
- make all operations have a (src, dest) format; at the moment there are
  some (notably operations on Stk) that have a (dest, src) format
Also:

(1) For the atomics the compiler started using a more pleasing pattern for temp register allocation where a platform layer would just allocate the temp register instead of returning a count of how many temp registers that were needed; together with the maybeFreeReg macros this cleaned up a lot of code.  We still have a few functions that use the old style: popcnt32NeedsTemp() etc return flags, which callers have to act on.  Should tidy this up.

(2) There's a little bit of excessive platform ifdeffery still.
Depends on: 1419034
Note, these patches sit on top of the stack frame refactoring in bug 1419034.
Blocks: 1420158
Note, rebasing + minor bug fixes, and one new patch.
Comment on attachment 8931317 [details]
Bug 1420104 - rabaldr, reorganize and tidy up.

https://reviewboard.mozilla.org/r/202472/#review209736

Nice.

::: js/src/wasm/WasmBaselineCompile.cpp:210
(Diff revision 2)
>  struct RegI64 : public Register64
>  {
>      RegI64() : Register64(Register64::Invalid()) {}
>      explicit RegI64(Register64 reg) : Register64(reg) {}
> +    bool valid() const { return *this != Invalid(); }
> +    bool invalid() const { return !valid(); }

slight preference for `isValid` and `isInvalid`, which 1. distinguish `invalid` better from the static `Invalid` and 2. indicate they return a boolean by the `is` prefix.

::: js/src/wasm/WasmBaselineCompile.cpp:3234
(Diff revision 2)
>  
> +    // The compiler depends on moveImm32() clearing the high bits of a 64-bit
> +    // register on 64-bit systems.
> +
> +    void moveImm32(int32_t v, RegI32 dest) {
> +        masm.mov(ImmWord(uint32_t(v)), dest);

For api consistency, rename `dest` to `r` here, or `r` to `dest` below x3?
Attachment #8931317 - Flags: review?(bbouvier) → review+
Comment on attachment 8931318 [details]
Bug 1420104 - rabaldr, fix operand order when loading from value stack.

https://reviewboard.mozilla.org/r/202474/#review209738

Sweet

::: js/src/wasm/WasmBaselineCompile.cpp:1875
(Diff revision 2)
> -    void loadConstI32(RegI32 r, Stk& src) {
> -        moveImm32(src.i32val(), r);
> +    void loadConstI32(Stk& src, RegI32 dest) {
> +        moveImm32(src.i32val(), dest);
>      }
>  
> -    void loadMemI32(RegI32 r, Stk& src) {
> -        fr.loadStackI32(r, src.offs());
> +    void loadMemI32(Stk& src, RegI32 dest) {
> +        fr.loadStackI32(dest, src.offs());

Hah, now got to do the same at the BaseStackFrame level...
Attachment #8931318 - Flags: review?(bbouvier) → review+
Comment on attachment 8931319 [details]
Bug 1420104 - rabaldr, fix operand order when loading from CPU stack and locals.

https://reviewboard.mozilla.org/r/202476/#review209740

(Thanks for using mozreview for these patches, it makes them really easy and quick to review)

::: js/src/wasm/WasmBaselineCompile.cpp:991
(Diff revision 2)
>          return localSize_;
>      }
>  
>      void zeroLocals(BaseRegAlloc& ra);
>  
> -    void loadLocalI32(RegI32 r, Local& local) {
> +    void loadLocalI32(Local& src, RegI32 dest) {

said before, but let's make this `const Local&` here and below
Attachment #8931319 - Flags: review?(bbouvier) → review+
Comment on attachment 8931320 [details]
Bug 1420104 - rabaldr, clean up temp register managements and some ifdefs.

https://reviewboard.mozilla.org/r/202478/#review209746

::: js/src/wasm/WasmBaselineCompile.cpp:3501
(Diff revision 2)
>  #else
>          pop2xI64(r0, r1);
>  #endif
>      }
>  
> -    bool rotate64NeedsTemp() const {
> +    RegI32 needRotate64Temp() {

(here and below) how about `maybeRotate64Temp` instead? (introduces a nice symmetry with `maybeFree`

::: js/src/wasm/WasmBaselineCompile.cpp:4195
(Diff revision 2)
> -    void needAtomicRMWTemps(AtomicOp op, MemoryAccessDesc* access, RegI32* tmp) {
> +    RegI32 needAtomicRMWTemp(AtomicOp op, MemoryAccessDesc* access) {
> +        RegI32 tmp;
>  #if defined(JS_CODEGEN_X86)
>          // Handled specially in atomicRMW
>          if (access->byteSize() == 1)
> -            return;
> +            return tmp;

Can we instead not have `tmp` and just return `RegI32::Invalid()` or `needI32()`?

::: js/src/wasm/WasmBaselineCompile.cpp:4271
(Diff revision 2)
> +        RegI64 tmp;
>  #if defined(JS_CODEGEN_X86)
>          MOZ_CRASH("Do not call on x86");
>  #elif defined(JS_CODEGEN_X64)
>          if (op != AtomicFetchAddOp && op != AtomicFetchSubOp)
> -            *tmp = needI64();
> +            tmp = needI64();

ditto
Attachment #8931320 - Flags: review?(bbouvier) → review+
Comment on attachment 8932419 [details]
Bug 1420104 - rabaldr, remove more ifdefs, simplify more.

https://reviewboard.mozilla.org/r/203462/#review209748

::: js/src/jit/MacroAssembler.h:1475
(Diff revision 1)
>                                 Register tmp)
>          DEFINED_ON(arm);
>  
>      // wasm specific methods, used in both the wasm baseline compiler and ion.
> -    void wasmTruncateDoubleToUInt32(FloatRegister input, Register output, Label* oolEntry) DEFINED_ON(x86, x64, arm, mips32, mips64);
> -    void wasmTruncateDoubleToInt32(FloatRegister input, Register output, Label* oolEntry) DEFINED_ON(x86_shared, arm, mips_shared);
> +    void wasmTruncateDoubleToUInt32(FloatRegister input, Register output, Label* oolEntry)
> +        DEFINED_ON(x86, x64, arm, arm64, mips32, mips64);

isn't there a better DEFINED_ON macro that means exactly this? PER_PLATFORM or something?

::: js/src/wasm/WasmBaselineCompile.cpp:3676
(Diff revision 1)
>          }
>      };
>  
>  #ifndef RABALDR_FLOAT_TO_I64_CALLOUT
> +    template<bool isUnsigned>
> +    MOZ_MUST_USE RegF64 needTempForFloatingToI64() {

It feels just a bit overkill to have a templated method for this, where it could just take a bool parameter instead (ditto for the caller).
Attachment #8932419 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
> Comment on attachment 8931320 [details]
> Bug 1420104 - rabaldr, clean up temp register managements and some ifdefs.
> 
> https://reviewboard.mozilla.org/r/202478/#review209746
> 
> ::: js/src/wasm/WasmBaselineCompile.cpp:3501
> (Diff revision 2)
> >  #else
> >          pop2xI64(r0, r1);
> >  #endif
> >      }
> >  
> > -    bool rotate64NeedsTemp() const {
> > +    RegI32 needRotate64Temp() {
> 
> (here and below) how about `maybeRotate64Temp` instead? (introduces a nice
> symmetry with `maybeFree`

I see your point, but (a) this follows a pattern that is already used many places in the compiler (needLoadTemps() etc) and (b) "need" is used as a flag for functions that may spill.  I'll think some more about this when I work on the RAII allocation wrappers.
(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> Comment on attachment 8932419 [details]
> Bug 1420104 - rabaldr, remove more ifdefs, simplify more.
> 
> ::: js/src/wasm/WasmBaselineCompile.cpp:3676
> (Diff revision 1)
> >          }
> >      };
> >  
> >  #ifndef RABALDR_FLOAT_TO_I64_CALLOUT
> > +    template<bool isUnsigned>
> > +    MOZ_MUST_USE RegF64 needTempForFloatingToI64() {
> 
> It feels just a bit overkill to have a templated method for this, where it
> could just take a bool parameter instead (ditto for the caller).

Heh, yes.  The caller can't avoid it due to macro magic further down but here we should avoid the template.
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/450a6a64ae2d
rabaldr, reorganize and tidy up. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b25e8dca0c
rabaldr, fix operand order when loading from value stack. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bc4d350ba3
rabaldr, fix operand order when loading from CPU stack and locals. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d399f9bb57
rabaldr, clean up temp register managements and some ifdefs.  r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb81fe6cac15
rabaldr, remove more ifdefs, simplify more. r=bbouvier
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: