Closed Bug 1313043 Opened 8 years ago Closed 8 years ago

Wasm baseline: Clean up platform ifdefs, part 1: the easy cases

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: lth, Assigned: lth)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #8804633 - Flags: review?(hv1989)
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?
Attachment #8804633 - Flags: review?(hv1989) → review+
(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/4be44008318449ec880e9d870281ec92612e6909
Bug 1313043 - create arm64 stubs for missing MacroAssembler instructions. r=me
Attached patch bug1313043-arm64-fixes.patch (obsolete) — Splinter Review
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.)
Attachment #8805076 - Attachment is obsolete: true
Attachment #8805470 - Flags: review?(bbouvier)
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.
Attachment #8805470 - Flags: review?(bbouvier) → review+
https://hg.mozilla.org/mozilla-central/rev/ec3a4e54003b
https://hg.mozilla.org/mozilla-central/rev/e64ad680a6c6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.