refactor: use bitflags-based Extensions for RISC-V decoder by manchangfengxu · Pull Request #40 · hust-open-atom-club/Robustone

manchangfengxu · 2025-11-20T11:08:59Z

Replace the old u32-based extension_masks with a bitflags::bitflags Extensions type in riscv::extensions
Update RiscVDecoder, RiscVHandler, and all RISC-V extension is_enabled implementations to use Extensions::contains()
Keep the existing RiscVExtensions type in riscv::arch for CLI-level configuration unchange

robustone-core/src/riscv/extensions/mod.rs

robustone-core/src/riscv/decoder.rs

Plucky923 · 2025-11-20T15:53:05Z

robustone-core/src/riscv/decoder.rs

    pub fn rv32gc() -> Self {
        Self::new(
            Xlen::X32,
-            extension_masks::I
-                | extension_masks::M
-                | extension_masks::A
-                | extension_masks::F
-                | extension_masks::C
-                | extension_masks::XTHEADCONDMOV,
+            Extensions::I
+                | Extensions::M
+                | Extensions::A
+                | Extensions::F
+                | Extensions::C
+                | Extensions::XTHEADCONDMOV,


The standard rv32gc instruction set does not include XThead. For the instruction sets defined by the manufacturer, we need to handle them in a separate function.

issue10 mentions that the final goal is to distinguish at the CLI level, but I'm not quite clear on how to refactor it yet. I need to discuss it with ChenMiao.

+thead Enables T-Head instruction sets. (only: RISC-V)

issue10 mentions that the final goal is to distinguish at the CLI level, but I'm not quite clear on how to refactor it yet. I need to discuss it with ChenMiao.

+thead Enables T-Head instruction sets. (only: RISC-V)

For this, in fact, I think the T-Head instruction is a single path and should come from RISC-V? The T-Head is a complete RISC-V instruction set — it should be at the same level as the standard instruction set, rather than being an extension of RISC-V. That’s exactly why I haven’t started working on it yet.

- Replace the old u32-based extension_masks with a bitflags::bitflags Extensions type in riscv::extensions - Update RiscVDecoder, RiscVHandler, and all RISC-V extension is_enabled implementations to use Extensions::contains() - Keep the existing RiscVExtensions type in riscv::arch for CLI-level configuration unchange Signed-off-by: manchangfengxu <manchangfengxu@openatom.club>

ChenMiaoi

LGTM!

ChenMiaoi · 2025-11-24T16:50:59Z

robustone-core/src/riscv/extensions/mod.rs

+    /// Bitflags representing enabled RISC-V extensions.
+    #[derive(Clone, Copy)]
+    pub struct Extensions: u32 {
+        const G             = Self::I.bits() | Self::M.bits() | Self::A.bits() | Self::F.bits() | Self::D.bits();


can you move the line Extensions::G below the single extension?

Plucky923 · 2025-11-25T02:39:03Z

robustone-core/src/riscv/extensions/mod.rs

+        const F             = 1 << 3;
+        const D             = 1 << 4;
+        const C             = 1 << 5;
+        const XTHEADCONDMOV = 1 << 6;


XTHEADCONDMOV is too long. Its meaning is T-head conditional move extension. The manufacturer name can be removed. Just call it CMOV to keep the name similar to x86. Then add annotations for explanation.
As a custom extension, this flag should not be placed together with the standard instruction set. Because there is a possibility of conflict among different custom extenstions. So you should define your own Extensions separately for T-Head.

Plucky923 · 2025-11-26T07:29:50Z

robustone-core/src/riscv/extensions/mod.rs

+    pub(crate) standard: StandardExtensions,
+    pub(crate) thead: THeadExtensions,
+}
+


In extensions module, This struct is called Extensions. So StandardExtensions, this Extensions is redundant. Just call it Standard

Okay, I will also change THeadExtensions to THead

Plucky923 · 2025-11-26T07:31:41Z

robustone-core/src/riscv/extensions/mod.rs

+    /// Enables all available T-Head custom extensions on this configuration.
+    pub fn set_thead(mut self) -> Self {
+        self.thead |= THeadExtensions::all();
+        self
+    }
+}


This function just crate a Extansions witch THead instruction set. It did not perform any operation on thead. So rename it to thead.

Plucky923 · 2025-11-26T07:33:06Z

robustone-core/src/riscv/extensions/mod.rs

+// RVA - Atomic Instructions
+// RVC - Compressed Instructions
+// RVD - Double-Precision Floating-Point
+// RVF - Single-Precision Floating-Point
+// RV32I/RV64I - Base Integer Instruction Set
+// RVM - Multiply and Divide Instructions


Why not remove these comments

Okay, I will delete it

Plucky923 · 2025-11-26T07:33:53Z

robustone-core/src/riscv/extensions/mod.rs

+// XuanTie custom extension modules
+// XTheadCondMov - Conditional Move Instructions


This comment is missing context, so remove it.

Plucky923 · 2025-11-26T08:15:10Z

robustone-core/src/riscv/extensions/thead/condmov.rs


 /// XTheadCondMov Conditional Move Extension
-pub struct XTheadCondMovExtension {
+pub struct CondMovExtension {


What about this？

Maybe it's not very good? Do I need to change it back?

You need to maintain consistency throughout the entire code style. In this module, Extension is redundant

extensions.standard.contains(Standard::A) This is more clear than before.
extensions.thead.contains(THead::CondMovExtension) This code contains two extensions, which are obviously redundant.

The CMOV constant is in THead(bitflags).

bitflags! { /// Bitflags representing enabled T-Head custom extensions. pub struct THead: u32 { /// Conditional move extension (XTheadCondMov). const CMOV = 1; } }

CondMovExtension is used to implement pub trait InstructionExtension: Sync
and other instruction sets under standard are currently in this form

impl InstructionExtension for RvaExtension{...}

manchangfengxu · 2025-11-26T09:28:49Z

    pub fn rv32gc() -> Self {
        Self {
            standard: Standard::G | Standard::C,
            thead: THead::empty(),
        }
    }

Should it be changed to

    pub fn rv32gc() -> Self {
        Self {
            standard: Standard::all(),
            thead: THead::empty(),
        }
    }

Plucky923 · 2025-11-26T14:43:08Z

robustone-core/src/riscv/extensions/mod.rs

        Box::new(RvdExtension::new()),
        Box::new(RvcExtension::new()),
-        Box::new(XTheadCondMovExtension::new()),
+        Box::new(CondMovExtension::new()),


The Extension here is also redundant. The Extension needs to be removed. use standard::Rva,Thead::CMov to improve readability.

- Move standard and T-Head extensions into standard/ and thead/ modules - Update decoder, handler and tests to use the new Extensions API Signed-off-by: manchangfengxu <manchangfengxu@openatom.club>

ChenMiaoi reviewed Nov 20, 2025

View reviewed changes

robustone-core/src/riscv/extensions/mod.rs Outdated Show resolved Hide resolved

robustone-core/src/riscv/decoder.rs Outdated Show resolved Hide resolved

robustone-core/src/riscv/decoder.rs Outdated Show resolved Hide resolved

Plucky923 reviewed Nov 21, 2025

View reviewed changes

manchangfengxu force-pushed the feat/extension branch from 1fd0516 to 08cb4c5 Compare November 24, 2025 12:31

ChenMiaoi approved these changes Nov 24, 2025

View reviewed changes

ChenMiaoi reviewed Nov 24, 2025

View reviewed changes

Plucky923 reviewed Nov 25, 2025

View reviewed changes

manchangfengxu force-pushed the feat/extension branch from 6a1040d to 60b4290 Compare November 26, 2025 01:19

Plucky923 reviewed Nov 26, 2025

View reviewed changes

manchangfengxu force-pushed the feat/extension branch from 60b4290 to 214354a Compare November 26, 2025 08:08

Plucky923 reviewed Nov 26, 2025

View reviewed changes

feat: split RISC-V extensions into standard and thead modules

e5b66d4

- Move standard and T-Head extensions into standard/ and thead/ modules - Update decoder, handler and tests to use the new Extensions API Signed-off-by: manchangfengxu <manchangfengxu@openatom.club>

manchangfengxu force-pushed the feat/extension branch from 214354a to e5b66d4 Compare November 26, 2025 15:10

Plucky923 approved these changes Nov 27, 2025

View reviewed changes

ChenMiaoi merged commit 61fb16a into hust-open-atom-club:main Nov 27, 2025
8 checks passed

manchangfengxu deleted the feat/extension branch November 27, 2025 13:47

		// XuanTie custom extension modules
		// XTheadCondMov - Conditional Move Instructions

Uh oh!

Conversation

manchangfengxu commented Nov 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenMiaoi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manchangfengxu commented Nov 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants