Closed Bug 2056833 Opened 3 months ago Closed 3 months ago

Fix and improve Atomic operations on riscv64

Categories

(Core :: JavaScript Engine: JIT, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
155 Branch
Tracking Flags:
Tracking Status
firefox155 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(13 external files)

Improvements:

  • Use AMO instructions instead of LR/SC loops where possible, because it needs fewer instructions and matches what Clang/GCC/V8 is using.
  • Using AMO instructions also means we don't need to worry about constraining LR/SC loops (see below).

Fixes:

  • Ensure LR/SC loops are always constrained, because unconstrained loops are allowed to never succeed.
  • Wasm Atomic operations are missing index zero-extension
  • Wasm compare-exchange in baseline doesn't sign-extend the expected value.

These were probably needed at some point to explicitly cast from
ScratchRegisterScope to Register, but are now no longer necessary.

Add zext_b and its companion method ZeroExtendByte. Then replace existing
callers to use either zext_b or ZeroExtendByte, depending on the surrounding
code.

Drive-by change:

  • Remove xlen uses in SignExtendByte, because we only support RV64.

Add ZeroExtendShort which emits zext.h if the Zbb extension is available.

Drive-by change:

  • Remove xlen uses in SignExtendByte, because we only support RV64.

Remove the old Simulator::amo() method because it didn't perform any
synchronization. Add AtomicMemoryHelper similar to upstream V8, but
only support SeqCst (Release+Acquire) ordering, so we can reuse the
atomic primitives from AtomicOperations.

Remove support for AMO min/max instructions, because there are no matching
primitives from AtomicOperations.

Use AMO instructions amoswap.w/d if possible, otherwise fallback to the
existing LR/SC loop. Within the LR/SC loop ensure all requirements for
"constrained LR/SC loop" are fulfilled.

The "A" extension consists of the Zalrsc and Zaamo extensions, and the RV64G
base ISA requires support for the "A" extension, so it's always okay to emit
AMO instructions. This also matches Clang/GCC when using SeqCst std::atomic
operations and what V8 is using.

Adds new helpers Atomic{Offset,Mask,ShiftToOffset,ExtendResult} which are
reused in the next parts to avoid duplicating the same code multiple times.

64-bit Atomic fetch operations can be implemented using a single AMO
instructions. The additional temp register is no longer needed.

Use different implementation strategies based on the bit-width and operation:

  1. 32-bit operations can always use AMO instructions.
  2. 8/16-bit bitwise operations can be be implemented using 32-bit AMO instructions.
  3. For Add/Sub we need to keep using a LR/SC loop.

Merges AtomicFetchOp and AtomicEffectOp into a single AtomicFetchOrEffectOp
function to reduce code duplication. "Effect" operations pass zero_reg as the
output register.

Change the implementation to follow "constrained LR/SC loop" constraints and
improve it to generate fewer instructions (based on what Clang is using for
C++ std::atomic operations).

When AMO instructions are used, no input register is read after writing to the
output register, so we can use at-start allocations here.

Drive-by changes:

  • Remove const for values to match other code. const also doesn't give any
    benefits, because the methods are short enough so it's easy to see that no
    modifications take place.
  • Use more auto*.

After parts 2-3, xlen is now only used by the simulator.

The Zba extension instructions add.uw and sh{1,2,3}add.uw are exactly what's
needed for Wasm load/store instructions with an Int32 index.

This patch adds enum ZeroExtendIndex to control when the index member of
BaseIndex needs to be zero-extended. "CodeGenerator-riscv64.cpp" and
"WasmBCMemory.cpp" now pass ZeroExtendIndex to the wasm load/store methods
instead of manually zero-extending the index.

This approach ensures better code generation and is also needed for part 13 to
add missing index zero-extension for Atomics operations.

Compare-Exchange is implemented using a LR/SC loop, so the expected value needs
to be sign-extended to a word to ensure other bits in the high word are ignored.

The test case in part 13 fails without this change.

Pass ZeroExtend32To64 to properly zero-extend the index for Atomics
operations. Otherwise we end up adding the sign-extended index, for example
0xFFFFFFFF80000000 instead of 0x80000000.

Pushed by andre.bargull@gmail.com: https://github.com/mozilla-firefox/firefox/commit/ad2b01208c49 https://hg.mozilla.org/integration/autoland/rev/76721b4f58d5 Part 1: Remove unnecessary Register constructor calls. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/f29f9eb1ec37 https://hg.mozilla.org/integration/autoland/rev/40309fa2a158 Part 2: Add zext_b pseudo-instruction and ZeroExtendByte. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/7e44e9d45e62 https://hg.mozilla.org/integration/autoland/rev/2fcb0b9124ad Part 3: Add ZeroExtendShort. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/1382197918d8 https://hg.mozilla.org/integration/autoland/rev/e951a4bf175c Part 4: Fix simulation of AMO instructions. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/986751148144 https://hg.mozilla.org/integration/autoland/rev/fbc37029f083 Part 5: Use AMO instructions for AtomicExchange(64). r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/bfcf961c5588 https://hg.mozilla.org/integration/autoland/rev/20702b466582 Part 6: Use AMO instructions for AtomicFetchOp64. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/a025edc0d0b8 https://hg.mozilla.org/integration/autoland/rev/35bdde524cc1 Part 7: Use AMO instructions for AtomicFetch/EffectOp. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/7a3e245be16a https://hg.mozilla.org/integration/autoland/rev/d6447c3bd5bc Part 8: Change CompareExchange(64) to follow "constrained LR/SC loop" constraints. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/6ad834629205 https://hg.mozilla.org/integration/autoland/rev/5f23707971ed Part 9: Use at-start allocations when 64-bit AMO instructions are used. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/914d5092836e https://hg.mozilla.org/integration/autoland/rev/1e106724a415 Part 10: Move xlen to Simulator. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/0d769b288910 https://hg.mozilla.org/integration/autoland/rev/26b9c0e38800 Part 11: Generate better code for wasm load/stores for riscv64. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/d3c11e45e508 https://hg.mozilla.org/integration/autoland/rev/7963a854d65c Part 12: Add missing move64To32 for compare-exchange. r=spidermonkey-reviewers,jandem https://github.com/mozilla-firefox/firefox/commit/f8a24e6488fc https://hg.mozilla.org/integration/autoland/rev/b1f18bf91233 Part 13: Pass ZeroExtend32To64 to Wasm Atomic operations. r=spidermonkey-reviewers,jandem
See Also: → 2061309
See Also: → 2061654
QA Whiteboard: [qa-triage-done-c156/b155]
Regressions: 2091045
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: