LibDisassembly: RISC-V disassembly 4-2/4-5: Disassemble extensions FD (floating-point), A (atomic) and Zicsr (CSRs) #25836

kleinesfilmroellchen · 2025-03-25T20:20:55Z

This second PR in the collection deals with all the remaining non-priviledged extensions we need right now. I encourage anyone with three years of free time to tackle V :3

LibDisassembly: Add RISC-V Zicsr extension decoding

Note that CSR pretty-printing is not yet implemented, since we aim to establish shared CSR utilities in AK in the near future.

LibDisassembly: Add RISC-V FD extension decoding

Again, no point in splitting these two since the orthagonal instruction set makes supporting both in common abstractions convenient.

LibDisassembly: Add RISC-V A extension decoding

Note that CSR pretty-printing is not yet implemented, since we aim to establish shared CSR utilities in AK in the near future.

Again, no point in splitting these two since the orthagonal instruction set makes supporting both in common abstractions convenient.

Hendiadyoin1

My ketchup, you already have the mustard
(Also not sure why I reviewed the commits in reverse)

Hendiadyoin1 · 2025-03-25T23:03:10Z

Userland/Libraries/LibDisassembly/riscv64/A.cpp

+        auto is_sc = (raw_parts.funct7 & (1 << 2)) > 0;
+        return adopt_own(*new (nothrow) LoadReserveStoreConditional(is_sc ? LoadReserveStoreConditional::Operation::StoreConditional : LoadReserveStoreConditional::Operation::LoadReserve, is_acquire, is_release, width, raw_parts.rs1, raw_parts.rs2, raw_parts.rd));
+    }
+    case 0b00001:


I am once again asking you to make this the enum values you are eventually/essentially assigning, so this becomes a cast-or-rejection pattern
(Seems like this did not make the cut for the the arithmetic instructions in G?)

Hendiadyoin1 · 2025-03-25T23:03:40Z

Userland/Libraries/LibDisassembly/riscv64/FD.cpp

+
+    auto operation = FloatArithmeticInstruction::Operation::Add;
+    switch (raw_parts.funct7 & ~0b11) {
+    case 0b0000000:


Same comment as in A

Hendiadyoin1 · 2025-03-25T23:09:45Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.h

+    ErrorOr<void> format(FormatBuilder& builder, Disassembly::RISCV64::CSRInstruction::Operation op)
+    {
+        auto op_name = ""sv;
+        switch (op) {


Maybe sprinkle some using/using enums in the formatters
The namespace lookup chains assume alarming proportions

spholz · 2025-03-26T09:51:57Z

Userland/Libraries/LibDisassembly/riscv64/Zicsr.cpp

+    auto is_immediate = (raw_parts.funct3 & 0b100) > 0;
+    auto operation = CSRInstruction::Operation::ReadWrite;
+
+    switch (raw_parts.funct3 & 0b11) {


Can we add

case 0b00: VERIFY_NOT_REACHED();

here (just to be safe)? SYSTEM instructions with funct3 == 0 shouldn't be CSR instructions afaict.

spholz · 2025-03-26T10:00:04Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

@@ -172,6 +172,26 @@ String InstructionFetchFence::mnemonic() const
    return "fence.i"_string;
 }

+String CSRImmediateInstruction::to_string(DisplayStyle display_style, u32, Optional<SymbolProvider const&>) const
+{
+    return MUST(String::formatted("{:10} {}, {}, {:d}", mnemonic(), format_register(destination_register(), display_style), immediate(), csr()));


CSR immediate instructions are written as csrrwi rd, csr, uimm.
Also, CSR addresses are typically formatted as hex.

spholz · 2025-03-26T10:01:31Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

+
+String CSRRegisterInstruction::to_string(DisplayStyle display_style, u32, Optional<SymbolProvider const&>) const
+{
+    return MUST(String::formatted("{:10} {}, {}, {:d}", mnemonic(), format_register(destination_register(), display_style), format_register(source_register(), display_style), csr()));


CSR register instructions are written as csrrw rd, csr, rs.
Also, CSR addresses are typically formatted as hex.

spholz · 2025-03-29T17:14:57Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

@@ -147,6 +271,16 @@ String MemoryLoad::mnemonic() const
    return MUST(String::formatted("l{}", m_width));
 }

+String FloatMemoryLoad::to_string(DisplayStyle display_style, u32, Optional<SymbolProvider const&>) const
+{
+    return MUST(String::formatted("fl{:8} {}, {:#03x}({})", memory_width(), format_register(destination_register(), display_style), immediate(), format_register(base(), display_style)));


You use mnemonic() in the format string everywhere except here and in FloatMemoryStore::to_string.

spholz · 2025-03-29T17:35:44Z

Userland/Libraries/LibDisassembly/riscv64/FD.h

+    RoundingMode m_rounding_mode;
+};
+
+class FloatRTypeInstruction : public FloatComputationInstruction {


Why do you not derive from this class in some of the R-Type F/D instruction implementations?

Tbh I think the inheritance hierarchy is a bit overengineered in general.
(not a blocker though)

spholz · 2025-03-30T12:00:55Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

@@ -352,4 +352,25 @@ String Fence::mnemonic() const
    VERIFY_NOT_REACHED();
 }

+String AtomicMemoryOperation::to_string(DisplayStyle display_style, u32, Optional<SymbolProvider const&>) const
+{
+    return MUST(String::formatted("{:10} {}, {}, {}", mnemonic(), format_register(destination_register(), display_style), format_register(source_register_1(), display_style), format_register(source_register_2(), display_style)));


amo{} rd, rs2, (rs1) is the correct syntax, not amo{} rd, rs1, rs2.

spholz · 2025-03-30T12:06:54Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

+
+String LoadReserveStoreConditional::to_string(DisplayStyle display_style, u32, Optional<SymbolProvider const&>) const
+{
+    return MUST(String::formatted("{:10} {}, {} ({})", mnemonic(), format_register(destination_register(), display_style), format_register(source_register_2(), display_style), format_register(source_register_1(), display_style)));


lr only has one source operand: rs1 as the address.

spholz · 2025-03-30T12:09:28Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

+
+String AtomicMemoryOperation::mnemonic() const
+{
+    return MUST(String::formatted("amo{}.{}{}{}", m_operation, MemoryAccessMode { width(), Signedness::Signed }, is_acquire() ? ".aq"sv : ""sv, is_release() ? ".rl"sv : ""sv));


.aqrl is the correct syntax for both acquire and release.

spholz · 2025-03-30T12:09:37Z

Userland/Libraries/LibDisassembly/riscv64/Formatting.cpp

+String LoadReserveStoreConditional::mnemonic() const
+{
+    auto operation = m_operation == Operation::LoadReserve ? "lr"sv : "sc"sv;
+    return MUST(String::formatted("{}.{}{}{}", operation, MemoryAccessMode { width(), Signedness::Signed }, is_acquire() ? ".aq"sv : ""sv, is_release() ? ".rl"sv : ""sv));


spholz · 2025-03-30T12:10:37Z

Userland/Libraries/LibDisassembly/riscv64/A.h

+class LoadReserveStoreConditional : public AtomicOperation {
+public:
+    enum class Operation : bool {
+        LoadReserve,
+        StoreConditional,
+    };


s/LoadReserve/LoadReserved

kleinesfilmroellchen added 3 commits March 25, 2025 21:02

LibDisassembly: Add RISC-V Zicsr extension decoding

3a127f9

Note that CSR pretty-printing is not yet implemented, since we aim to establish shared CSR utilities in AK in the near future.

LibDisassembly: Add RISC-V FD extension decoding

16a7ece

Again, no point in splitting these two since the orthagonal instruction set makes supporting both in common abstractions convenient.

LibDisassembly: Add RISC-V A extension decoding

0de3973

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Mar 25, 2025

Hendiadyoin1 reviewed Mar 25, 2025

View reviewed changes

spholz reviewed Mar 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LibDisassembly: RISC-V disassembly 4-2/4-5: Disassemble extensions FD (floating-point), A (atomic) and Zicsr (CSRs) #25836

LibDisassembly: RISC-V disassembly 4-2/4-5: Disassemble extensions FD (floating-point), A (atomic) and Zicsr (CSRs) #25836

kleinesfilmroellchen commented Mar 25, 2025

Hendiadyoin1 left a comment

Hendiadyoin1 Mar 25, 2025

Hendiadyoin1 Mar 25, 2025

Hendiadyoin1 Mar 25, 2025

spholz Mar 26, 2025

spholz Mar 26, 2025

spholz Mar 26, 2025

spholz Mar 29, 2025

spholz Mar 29, 2025

spholz Mar 29, 2025 •

edited

Loading

spholz Mar 30, 2025

spholz Mar 30, 2025

spholz Mar 30, 2025

spholz Mar 30, 2025

spholz Mar 30, 2025

LibDisassembly: RISC-V disassembly 4-2/4-5: Disassemble extensions FD (floating-point), A (atomic) and Zicsr (CSRs) #25836

Are you sure you want to change the base?

LibDisassembly: RISC-V disassembly 4-2/4-5: Disassemble extensions FD (floating-point), A (atomic) and Zicsr (CSRs) #25836

Conversation

kleinesfilmroellchen commented Mar 25, 2025

LibDisassembly: Add RISC-V Zicsr extension decoding

LibDisassembly: Add RISC-V FD extension decoding

LibDisassembly: Add RISC-V A extension decoding

Hendiadyoin1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spholz Mar 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spholz Mar 29, 2025 •

edited

Loading