There are now much better MacroAssembler abstractions, and some of the patterns in the compiler are much clearer, than when work on the compiler started. It is easy to clean up many ifdefs.

(This will probably affect the MIPS port.)

It is easiest to read this patch from the bottom and up, as the uses further down motivate the introduction of new and removal of old abstractions above. Many platform ifdefs are replaced by uses of MacroAssembler abstractions, sometimes with new (cross-platform, with per-platform implementations) helpers. Patterns are exploited, eg, all the shift and rotate instructions pop their arguments in the same way, so that can be commoned.

Comment on attachment 8804633 [details] [diff] [review] bug1313043-baseline-ifdef-removal.patch Review of attachment 8804633 [details] [diff] [review]: ----------------------------------------------------------------- I really liked reading this! ::: js/src/asmjs/WasmBaselineCompile.cpp @@ +968,5 @@ > + return r.reg.low; > +#endif > + } > + > + Register highPartMaybe(RegI64 r) { maybeHighPart() @@ +976,5 @@ > + return r.reg.high; > +#endif > + } > + > + void clearHighPartMaybe(RegI64 r) { maybeClearHighPart @@ +2636,5 @@ > + > + void pop2xI64ForShiftOrRotate(RegI64* r0, RegI64* r1) { > +#if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64) > + needI32(specific_ecx); > + *r1 = fromI32(specific_ecx); I still find 'fromI32' a strange name for this. Should we try to find a better name? extendRegI32toI64 or fromI32toI64?

(In reply to Hannes Verschore [:h4writer] from comment #3) > Comment on attachment 8804633 [details] [diff] [review] > bug1313043-baseline-ifdef-removal.patch > > Review of attachment 8804633 [details] [diff] [review]: > ----------------------------------------------------------------- > > > @@ +2636,5 @@ > > + > > + void pop2xI64ForShiftOrRotate(RegI64* r0, RegI64* r1) { > > +#if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64) > > + needI32(specific_ecx); > > + *r1 = fromI32(specific_ecx); > > I still find 'fromI32' a strange name for this. Should we try to find a > better name? > extendRegI32toI64 or fromI32toI64? I agree, this is not the right name. I decided to go for 'widenI32' which captures it pretty well and is short enough.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d72443b3ab664c1c7fe039d58e5dee1f3e672fa Bug 1313043 - wasm baseline: remove many ifdefs. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/4be44008318449ec880e9d870281ec92612e6909 Bug 1313043 - create arm64 stubs for missing MacroAssembler instructions. r=me

Had to make up for some missing implementations on ARM64 and landed this patch containing stubs. Will file new bug to follow up with the implementation work.

(In reply to Lars T Hansen [:lth] from comment #7) > Created attachment 8805076 [details] [diff] [review] > bug1313043-arm64-fixes.patch > > Had to make up for some missing implementations on ARM64 and landed this > patch containing stubs. Will file new bug to follow up with the > implementation work. New bug is bug 1313336.

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0c3bb88db2 Backed out changeset 4be440083184 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5bff02eebd Backed out changeset 2d72443b3ab6 for arm bustage

Backed out for style violations, so let's get a proper review. I need these implementations: the baseline compiler fails arm64 compilation on inbound because it uses the abstractions to get rid of ifdefs, see the other patch. (It's annoying and weird that we're testing a Tier-3 platform on CI.) I've filed bug 1313336 to actually implement these, though that is likely not to happen for a while unless Ion starts using these new abstractions. (I'll look into implementations for "None" separately.)

Comment on attachment 8805470 [details] [diff] [review] bug1313043-arm64-fixes-v2.patch Review of attachment 8805470 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! If you're in the mood, rs=me for the mips/none stubs too. ::: js/src/jit/MacroAssembler.h @@ +926,5 @@ > > // On x86_shared, temp may be Invalid only if the chip has the POPCNT instruction. > // On ARM, temp may never be Invalid. > inline void popcnt32(Register src, Register dest, Register temp) > + DEFINED_ON(x86_shared, arm, arm64, mips_shared); This can be reduced to PER_SHARED_ARCH.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3a4e54003b3b96eb22a7afcceecef36592d3d8 Bug 1313043 - Create arm64 stubs for missing MacroAssembler instructions. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/e64ad680a6c6fe7e64f8fce2fc7ee07555de6957 Bug 1313043 - wasm baseline: remove many ifdefs. r=h4writer

https://hg.mozilla.org/mozilla-central/rev/ec3a4e54003b https://hg.mozilla.org/mozilla-central/rev/e64ad680a6c6