Closed Bug 2039343 Opened 6 months ago Closed 5 months ago

Improve riscv64 instruction class

Categories

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

Product:
Core
Component:
JavaScript Engine: JIT
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

(26 external files)

Improve the riscv64 Instruction class and code using Instruction:

  • Move all manual Instr bitwise manipulations into InstructionBase.
  • Use fewer reinterpret_cast for Instruction.
  • Avoid duplicated code to split immediates into smaller bit ranges.
  • And more clean-ups..

It's confusing that the base assemblers were placed in the "extension" directory.

This will allow to reuse them in InstructionBase in a later patch in this stack.

I've used the 2012 copyright notice matching both V8 source files (common/globals.h and utils/utils.h).

Drive-by change:

  • Remove unused EmbeddedVector methods which don't compile anyway.
  • Move constant and comments from Constant-riscv64.h to correct locations.

Moving Instruction into a separate header will allow to add more convenience
methods in later patches, because it's now decoupled from the "constants" headers.

Move trivial methods into the header file to ensure they can get fully inlined.

Drive-by change:

  • Add missing definitions for Funct7Value and Funct2Value.
  • Add missing instruction type check in Funct3Value.

Directly inline InstructionGetters into InstructionBase to simplify the class
inheritance structure.

This is no longer needed since C++11.

This follows ARM64 instead of ARM32.

Also start with replacing reinterpret_cast with SetInstructionBits.

Prefer Instruction::At to keep reinterpret_cast in a single place.

Use InstructionBits() to get instruction bits and SetInstructionBits() to set
instruction bits instead of using reinterpret_cast.

Instead of duplicating the same code in multiple places.

Instead of using static methods on the assembler to determine the instruction kind,
add them as getters to InstructionBase.

Use the existing getter functions from InstructionBase instead of manually
extracting the immediate values.

Remove dead code so we don't have to update it in the next part.

Add proper setter methods instead of manually constructing the instruction bit encoding.

Also move get_pc_as closer to get_pc to make it more discoverable.

This avoids more reinterpret_casts and further reduces the calls to InstructionBits().

It's RO_FSGNJX instead of RO_FSQNJX.

The same code was repeated in multiple places, sometimes in a slightly different form.

is_int32(int32) is trivially true.

The comments were copied from ARM64, but not updated for RISCV64.

Pushed by andre.bargull@gmail.com: https://github.com/mozilla-firefox/firefox/commit/6e104bd2ad57 https://hg.mozilla.org/integration/autoland/rev/edaada7978cf Part 1: Move base assemblers into "base" directory. r=jandem https://github.com/mozilla-firefox/firefox/commit/90ec4173d11a https://hg.mozilla.org/integration/autoland/rev/240401ef9876 Part 2: Move integer width from base-assembler-riscv.h into a separate header. r=jandem https://github.com/mozilla-firefox/firefox/commit/a175da49ac60 https://hg.mozilla.org/integration/autoland/rev/b2bf3ebfe59d Part 3: Move vector classes into new "base" directory. r=jandem https://github.com/mozilla-firefox/firefox/commit/93c3a7024b44 https://hg.mozilla.org/integration/autoland/rev/5ebf7531613d Part 4: Move Instruction class into a separate header. r=jandem https://github.com/mozilla-firefox/firefox/commit/be53ea124d6e https://hg.mozilla.org/integration/autoland/rev/fd611af48a61 Part 5: Move trivial Instruction methods to header. r=jandem https://github.com/mozilla-firefox/firefox/commit/3111deaff090 https://hg.mozilla.org/integration/autoland/rev/8ea81d2ba65d Part 6: Remove InstructionGetters. r=jandem https://github.com/mozilla-firefox/firefox/commit/ab33385c78b5 https://hg.mozilla.org/integration/autoland/rev/94119e1cb190 Part 7: Remove this-> prefix from calls. r=jandem https://github.com/mozilla-firefox/firefox/commit/34e7fed554ac https://hg.mozilla.org/integration/autoland/rev/669430f3c42d Part 8: Make sext and zext proper functions and use them more. r=jandem https://github.com/mozilla-firefox/firefox/commit/4d1e90d0a31f https://hg.mozilla.org/integration/autoland/rev/2276bab5ea05 Part 9: Rename editSrc to getInstructionAt. r=jandem https://github.com/mozilla-firefox/firefox/commit/ea39ade67046 https://hg.mozilla.org/integration/autoland/rev/54c69f8069bc Part 10: Prefer Instruction::At to convert to Instruction. r=jandem https://github.com/mozilla-firefox/firefox/commit/e6b66ab514e0 https://hg.mozilla.org/integration/autoland/rev/fa7e23f8f52c Part 11: Remove reinterpret_cast for Instruction bits. r=jandem https://github.com/mozilla-firefox/firefox/commit/fd8d5d90304f https://hg.mozilla.org/integration/autoland/rev/e4197f63517f Part 12: Add setters for instruction immediate values. r=jandem https://github.com/mozilla-firefox/firefox/commit/1884ad8f8dc3 https://hg.mozilla.org/integration/autoland/rev/4a6bf537155c Part 13: Use instruction getters to determine instruction kind. r=jandem https://github.com/mozilla-firefox/firefox/commit/2eac7b1dba4e https://hg.mozilla.org/integration/autoland/rev/2c5e0a33a0e2 Part 14: Use Instruction getters instead of Assembler static methods. r=jandem https://github.com/mozilla-firefox/firefox/commit/b9541a9411ae https://hg.mozilla.org/integration/autoland/rev/5e947ec3efb8 Part 15: Remove unused is_internal parameter. r=jandem https://github.com/mozilla-firefox/firefox/commit/4cf21a62f1d8 https://hg.mozilla.org/integration/autoland/rev/1fcedf032075 Part 16: Remove unused trampoline parameter. r=jandem https://github.com/mozilla-firefox/firefox/commit/d9a84e0607c3 https://hg.mozilla.org/integration/autoland/rev/7bc140b7dbc5 Part 17: Use Instruction setters instead of Assembler static methods. r=jandem https://github.com/mozilla-firefox/firefox/commit/16ad73ec95fa https://hg.mozilla.org/integration/autoland/rev/97a06f94ee41 Part 18: Use Simulator::get_pc_as instead of manual reinterpret_cast. r=jandem https://github.com/mozilla-firefox/firefox/commit/2abb9999c668 https://hg.mozilla.org/integration/autoland/rev/21055d8100f6 Part 19: Directly pass Instruction* to disassembler. r=jandem https://github.com/mozilla-firefox/firefox/commit/89eeb999445a https://hg.mozilla.org/integration/autoland/rev/a1f95b23891e Part 20: Fix typo in RO_FSGNJX. r=jandem https://github.com/mozilla-firefox/firefox/commit/b374cb5b3700 https://hg.mozilla.org/integration/autoland/rev/15ecfa3ea309 Part 21: Add ToHigh20Low12 to split an int32 immediate. r=jandem https://github.com/mozilla-firefox/firefox/commit/c6f4a315a511 https://hg.mozilla.org/integration/autoland/rev/886886449fe7 Part 22: Add ToImmPtrParts and ToImm64Parts. r=jandem https://github.com/mozilla-firefox/firefox/commit/1b2c58e72a0a https://hg.mozilla.org/integration/autoland/rev/a04e5b6d5530 Part 23: Always use kInstrSize for instruction size length. r=jandem https://github.com/mozilla-firefox/firefox/commit/68fc4ff3447a https://hg.mozilla.org/integration/autoland/rev/ec137022ae7c Part 24: Remove is_int32 checks which are always true. r=jandem https://github.com/mozilla-firefox/firefox/commit/f13eb121045d https://hg.mozilla.org/integration/autoland/rev/e318f667b9f7 Part 25: Use zero as placeholder in callWithPatch. r=jandem https://github.com/mozilla-firefox/firefox/commit/b92706cd263d https://hg.mozilla.org/integration/autoland/rev/01ec19e20414 Part 26: Update comments for ImmBranchRangeType. r=jandem
Blocks: sm-riscv64
Severity: -- → N/A
Priority: -- → P2

https://hg.mozilla.org/mozilla-central/rev/edaada7978cf
https://hg.mozilla.org/mozilla-central/rev/240401ef9876
https://hg.mozilla.org/mozilla-central/rev/b2bf3ebfe59d
https://hg.mozilla.org/mozilla-central/rev/5ebf7531613d
https://hg.mozilla.org/mozilla-central/rev/fd611af48a61
https://hg.mozilla.org/mozilla-central/rev/8ea81d2ba65d
https://hg.mozilla.org/mozilla-central/rev/94119e1cb190
https://hg.mozilla.org/mozilla-central/rev/669430f3c42d
https://hg.mozilla.org/mozilla-central/rev/2276bab5ea05
https://hg.mozilla.org/mozilla-central/rev/54c69f8069bc
https://hg.mozilla.org/mozilla-central/rev/fa7e23f8f52c
https://hg.mozilla.org/mozilla-central/rev/e4197f63517f
https://hg.mozilla.org/mozilla-central/rev/4a6bf537155c
https://hg.mozilla.org/mozilla-central/rev/2c5e0a33a0e2
https://hg.mozilla.org/mozilla-central/rev/5e947ec3efb8
https://hg.mozilla.org/mozilla-central/rev/1fcedf032075
https://hg.mozilla.org/mozilla-central/rev/7bc140b7dbc5
https://hg.mozilla.org/mozilla-central/rev/97a06f94ee41
https://hg.mozilla.org/mozilla-central/rev/21055d8100f6
https://hg.mozilla.org/mozilla-central/rev/a1f95b23891e
https://hg.mozilla.org/mozilla-central/rev/15ecfa3ea309
https://hg.mozilla.org/mozilla-central/rev/886886449fe7
https://hg.mozilla.org/mozilla-central/rev/a04e5b6d5530
https://hg.mozilla.org/mozilla-central/rev/ec137022ae7c
https://hg.mozilla.org/mozilla-central/rev/e318f667b9f7
https://hg.mozilla.org/mozilla-central/rev/01ec19e20414

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
status-firefox153: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 153 Branch
QA Whiteboard: [qa-triage-done-c154/b153]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: