Closed Bug 2041359 Opened 5 months ago Closed 5 months ago

Avoid using riscv64 Operand class when not needed

Categories

(Core :: JavaScript Engine: JIT, task)

Product:
Core
Component:
JavaScript Engine: JIT
Platform:
RISCV64
Unspecified
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
153 Branch
Tracking Flags:
Tracking Status
firefox153 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(8 external files, 1 obsolete file)

Operand doesn't need to be used when it's directly unwrapped in the caller.

For example:

// MacroAssemblerRiscv64Compat
void unboxWasmAnyRefGCThingForGCBarrier(const Address& src, Register dest) {
  UseScratchRegisterScope temps(this);
  Register scratch = temps.Acquire();
  MOZ_ASSERT(scratch != dest);
  movePtr(ImmWord(wasm::AnyRef::GCThingMask), scratch);
  loadPtr(src, dest);
  ma_and(dest, dest, scratch);
}

// MacroAssemblerRiscv64
void ma_and(Register rd, Register rs, Operand rt) {
  if (rt.is_imm()) {
    if (is_int12(rt.immediate())) {
      andi(rd, rs, rt.immediate());
    } else {
      UseScratchRegisterScope temps(this);
      Register scratch = temps.Acquire();
      ma_li(scratch, rt.immediate());
      and_(rd, rs, scratch);
    }
  } else {
    MOZ_ASSERT(rt.is_reg());
    and_(rd, rs, rt.rm());
  }
}

It's inefficient to first wrap the Register in Operand when ma_and will directly take a different code path for registers.

Directly calling and_ avoid this unnecessary indirection:

movePtr(ImmWord(wasm::AnyRef::GCThingMask), scratch);
loadPtr(src, dest);
and_(dest, dest, scratch);

Slightly better is to use ma_and with wasm::AnyRef::GCThingMask as the operand:

loadPtr(src, dest);
ma_and(dest, dest, ImmWord(wasm::AnyRef::GCThingMask));

And best is to directly call andi:

static_assert(is_int12(wasm::AnyRef::GCThingMask), "fits into andi");
loadPtr(src, dest);
andi(dest, dest, wasm::AnyRef::GCThingMask);

The "ma_<op>" methods accept Operand as the right-hand side operand. Operand
is either an immediate or a register. In the concrete "ma_<op>" methods there's
directly an if-else statement to give both operand types different code paths.

That means if Operand is always a Register, it's more efficient to directly
call the underlying assembler method, e.g. prefer addw instead of ma_add32.

If Operand is always a small immediate, let's also prefer directly calling
addiw instead of ma_add32.

This avoids some unnecessary indirections and prepares for changing ma_<op>
to only accept immediates in a later part of this patch stack.

This were either already unused or are unused after the changes from part 1.

The previous code was excluding the range already covered through is_int12,
but both Clang and GCC can generate slightly better code when testing the
complete range from -4096 to 4094.

Common bit-masks can be performed using only two shifts.

More optimisations can be performed using Zbs extension instructions, but
that's for another bug.

After part 1, these methods are only called with immediates, so let's change the
argument from Operand to Imm32 resp. Imm64. Prefer Imm32/Imm64 instead
of int32_t/int64_t to avoid implicit conversions.

Also change "MacroAssembler-riscv64-inl.h" to call the ma_<op> methods to
improve code generation for small immediates.

Similar to the "ma_<op>" methods, also split the rotate instructions in two
different implementations for Register and Imm32 operands.

  • Operand::Tag::FREG and Operand::Tag::MEM aren't used and can be removed.
  • The CodeGeneratorRiscv64::ToOperand() methods are also unused.
  • Additionally make the Operand(Register) constructor explicit.

To match ma_and which also performs similar optimisations.

As a bonus add some extra optimisations when the "Zba" extension is available.

ma_mul32 and ma_mul64 don't perform the same optimisations, but that can
be fixed in another bug.

Pushed by andre.bargull@gmail.com: https://github.com/mozilla-firefox/firefox/commit/444db404059d https://hg.mozilla.org/integration/autoland/rev/2c4198280e00 Part 1: Avoid "ma_<op>" methods when direct assembler methods can be used. r=iain https://github.com/mozilla-firefox/firefox/commit/d2d479e28a84 https://hg.mozilla.org/integration/autoland/rev/721231268175 Part 2: Remove now unused "ma_<op>" methods. r=iain https://github.com/mozilla-firefox/firefox/commit/a3ded78631e9 https://hg.mozilla.org/integration/autoland/rev/c6fd398009c9 Part 3: Add is_two_int12. r=iain https://github.com/mozilla-firefox/firefox/commit/9c8008113ac0 https://hg.mozilla.org/integration/autoland/rev/253205d0442c Part 4: Add some optimisations to ma_and. r=iain https://github.com/mozilla-firefox/firefox/commit/9afa6f0f6950 https://hg.mozilla.org/integration/autoland/rev/9caac908dd89 Part 5: Change "ma_<op>" methods to only accept immediates. r=iain,csmantle https://github.com/mozilla-firefox/firefox/commit/be4db7fba145 https://hg.mozilla.org/integration/autoland/rev/fadd160656bf Part 6: Remove Operand from rotate instructions. r=iain https://github.com/mozilla-firefox/firefox/commit/f76f7d5a5908 https://hg.mozilla.org/integration/autoland/rev/f25c6543c2cc Part 7: Remove unused Operand methods and make constructors explicit. r=iain https://github.com/mozilla-firefox/firefox/commit/38e2eb62e996 https://hg.mozilla.org/integration/autoland/rev/7887e75461cb Part 8: Move multiply strength reduction to macro-assembler. r=iain https://github.com/mozilla-firefox/firefox/commit/52c40474bad7 https://hg.mozilla.org/integration/autoland/rev/24949d57b331 apply code formatting via Lando
Attachment #9589210 - Attachment is obsolete: false

Comment on attachment 9589210 [details]
Bug 2041359 - Added support for color swatches for attr() with type(<color>) specification. r=#devtools-reviewers,nchevobbe

Revision D302217 was moved to bug 2040912. Setting attachment 9589210 [details] to obsolete.

Attachment #9589210 - Attachment is obsolete: true
QA Whiteboard: [qa-triage-done-c154/b153]
Hardware: Unspecified → RISCV64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: