Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RISC-V for XuanTie C908 #115

Draft
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

thisisjube
Copy link

Adds an implementation for the RISC-V architecture on the XuanTie C908, initially supporting the RV32/64I and RV32/64M ISA extensions, with plans to include RV32/64V in the future. This PR also introduces a new structure for organizing files, classes, and related components. Additionally, it includes a mechanism to reduce the effort required to add new instructions by dynamically generating instruction classes from a simple list. Overall, the changes also aim to streamline the process of adding new architectures, making it faster and more straightforward.

thisisjube and others added 30 commits October 16, 2024 15:56
…for some instructions

The <w>-specifier allows having 32-bit and 64-bit versions of instructions in one class
which saves some work. While parsing, mnemonics are compared first to find the matching class
for an instruction. Since add<w> would not be identified as an add-instruction, we first must replace
<w> (if present) by the corresponding regex pattern and call re.match(srcline, pattern) afterward. A generic solution
for parsing specifiers which modify the mnemonic could be a future task.
The method is set as an attribute during each call of parser() atm. This assignment should happen at a proper place.
Copy link
Collaborator

@dop-amin dop-amin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial review:
The new mechanism of how instructions can be added saves a lot of time and your new structure makes this complicated process more accesible. That's great!

Overall, the code looks well written. There are a few left overs from aarch64 that should be removed and we should discuss how to go about the hints. See the comments for a bit more details (and some nit picky details).

@@ -1493,6 +1499,80 @@ def core(self, slothy):
slothy.config.sw_pipelining.optimize_postamble = False
slothy.optimize_loop("flt_radix4_fft_loop_start")

class RISC_VExample0(Example):
def __init__(self, var="", arch=RISC_V, target=Target_XuanTieC908):
name = "riscv_simple0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Have an actual "simple" and minimalistic example here instead of the full NTT code as this is also part of a separate example.

slothy.config.variable_size=True
slothy.config.constraints.stalls_first_attempt=32
slothy.config.inputs_are_outputs = True
slothy.config.outputs = ['x1', 'x2', 'x3', 'x4', 'x5', 'x6', 'x7', 'x8', 'x9', 'x10',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the current example as it is, there are too many outputs defined here. For example x10, which corresponds to a0 is used as an input in the input assembly and thus automatically gets marked as an input due to slothy.config.inputs_are_outputs = True.

Generally, we should try to keep this list as small as it needs to be.

// gap // ....................
str q3, [x0, #-16] // ...................*
// gap // ....................
// Instructions: 20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changes to this file should not be part of this PR?

"""This module defines a bunch of custom slothy exceptions."""


class FatalParsingException(Exception): # done
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "done" mean here?

self.args_out_restrictions.append(None)
self.args_out.append(hint_register_name(tag))

# self.arg_types_out.append(RegisterType.HINT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should revisit how hints are used here.


return src

flaglist = ["eq", "ne", "cs", "hs", "cc", "lo", "mi", "pl", "vs", "vc", "hi", "ls", "ge", "lt", "gt", "le"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all these options exist in RISC-V? Is it exactly the same as aarch64?


@staticmethod
def get_parser(pattern):
"""Build parser for given AArch64 instruction pattern"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RISC-V

@@ -0,0 +1,431 @@
from slothy.targets.riscv.instruction_core import Instruction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is deprecated, it can go?

def add_further_constraints(slothy):
if slothy.config.constraints.functional_only:
return
add_slot_constraints(slothy)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably these should be removed here.
I think the constraints are not added anyways because we're not working with vectors yet, so no harm is caused, but it's just wrong.



def add_st_hazard(slothy):
def is_vec_st_st_pair(inst_a, inst_b):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants