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

Zcmp ext #610

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Zcmp ext #610

wants to merge 13 commits into from

Conversation

rez5427
Copy link
Contributor

@rez5427 rez5427 commented Nov 4, 2024

I noticed #525, but I can't push to pr525.
I've temporarily implemented cm.push, cm.pop, cm.popret, and cm.popretz.
And add test files in riscv-tests riscv-software-src/riscv-tests#592,
This test file need to recompile the compiler see pr.
I run the test files on spike and this sail implementation, nothing wrong with it.
I can't find a better way to do with the assemblly.
This mapping zcmp_assembly_mapping use 200 lines to do this job.
I tried use function in assemblly, not working issue in here.
Tried to seperate {ra}-ish and stack_adj. seems like the rlist can only bind with one mapping.
Maybe sepertate rlist and spimm down below could save some lines.But it can't.
image

Copy link

github-actions bot commented Nov 4, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit eb47018. ± Comparison against base commit b8d1fa5.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

I didn't check the actual logic at all but the Sail code needs a bit of work so I've added some hints. It's non-exhaustive, but I'll take a second look when it's improved a bit. (Or if we need this extension then I'll finish it myself! :-D)

model/riscv_insts_zcmp.sail Show resolved Hide resolved
model/riscv_insts_zcmp.sail Outdated Show resolved Hide resolved
model/riscv_insts_zcmp.sail Outdated Show resolved Hide resolved
0b1101 => 48,
0b1110 => 48,
0b1111 => 64,
_ => internal_error(__FILE__, __LINE__, "Unsupported rlist: " ^ bits_str(rlist))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks reachable to me. Where's the code that ensures this is unreachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add the assert in every excute like this, I don't know this is ok or not.

function clause execute (CM_POPRETZ(rlist, spimm)) = {
  assert(rlist >=_u 0b0100);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Sail model shouldn't trigger an assert()or internal_error for any code that can actually execute. In this case your decoder lets you decode an instruction with any rlist value, so executing an instruction with rlist = 0b0000 will crash the model.

What would a real implementation do in that case?

model/riscv_insts_zcmp.sail Outdated Show resolved Hide resolved
model/riscv_insts_zcmp.sail Outdated Show resolved Hide resolved
let mask = zcmp_regmask(rlist);
let bytes = if sizeof(xlen) == 32 then 4 else 8;

var imm : int = width_bytes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do var imm : xlenbits = zeros(); and imm = imm - width_bytes.

I think imm is the wrong name too surely? Did you mean offset?


X(sp) = new_sp;

var imm : int = width_bytes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

model/riscv_insts_zcmp.sail Outdated Show resolved Hide resolved
model/riscv_insts_zcmp.sail Outdated Show resolved Hide resolved
@rez5427
Copy link
Contributor Author

rez5427 commented Nov 6, 2024

Add cm.mvsa01and cm.mva01s instructions

<-> 0b101 @ 0b011 @ r1s' : cregidx @ 0b11 @ r2s' : cregidx @ 0b10 if extensionEnabled(Ext_Zcmp)

function clause execute (CM_MVA01S(r1s', r2s')) = {
X(0b01010) = X(rns_to_regidx(r1s'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably clearer to use X(10)...

(0xf, 0b11, true, 32) <-> "{ra" ^ sep() ^ "s0-11}" ^ sep() ^ "112",
(0xf, 0b11, true, 64) <-> "{ra" ^ sep() ^ "s0-11}" ^ sep() ^ "128",
(0xf, 0b11, false,32) <-> "{ra" ^ sep() ^ "s0-11}" ^ sep() ^ "-112",
(0xf, 0b11, false,64) <-> "{ra" ^ sep() ^ "s0-11}" ^ sep() ^ "-128",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok in fairness this was pretty difficult and uses some advanced Sail features, but you can cut this down a lot. Here's my attempt. It also supports the x1, ... syntax.

I haven't tested it but I think it's quite a bit shorter (especially since you'll want to reuse rlist_stack_adj_base in the actual code), and easier to understand.

mapping zcmp_assembly_rlist : bits(4) <-> string = {
  0x4 <-> "ra",
  0x5 <-> "ra" ^ sep() ^ "s0",
  0x6 <-> "ra" ^ sep() ^ "s0-s1",
  0x7 <-> "ra" ^ sep() ^ "s0-s2",
  0x8 <-> "ra" ^ sep() ^ "s0-s3",
  0x9 <-> "ra" ^ sep() ^ "s0-s4",
  0xa <-> "ra" ^ sep() ^ "s0-s5",
  0xb <-> "ra" ^ sep() ^ "s0-s6",
  0xc <-> "ra" ^ sep() ^ "s0-s7",
  0xd <-> "ra" ^ sep() ^ "s0-s8",
  0xe <-> "ra" ^ sep() ^ "s0-s9",
  // To include s10, s11 must also be included.
  0xf <-> "ra" ^ sep() ^ "s0-s11",

  // Using X names instead of ABI names.
  0x4 <-> "x1",
  0x5 <-> "x1" ^ sep() ^ "x8",
  0x6 <-> "x1" ^ sep() ^ "x8-x9",
  0x7 <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18",
  0x8 <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x19",
  0x9 <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x20",
  0xa <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x21",
  0xb <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x22",
  0xc <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x23",
  0xd <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x24",
  0xe <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x25",
  0xf <-> "x1" ^ sep() ^ "x8-x9" ^ sep() ^ "x18-x27",
}

mapping negative_sign : bool <-> string = {
  false <-> "",
  true <-> "-",
}

// Note for the integer types we deal in 16s, so a value of 1 here means 16 bytes.
type stack_adj = range(1, if xlen == 32 then 7 else 10)
type stack_adj_base = range(1, if xlen == 32 then 4 else 7)

mapping stack_adj_str : string <-> stack_adj = {
  "16" <-> 1,
  "32" <-> 2,
  "48" <-> 3,
  "64" <-> 4,
  "80" <-> 5,
  "96" <-> 6,
  "112" <-> 7,
  "128" if xlen == 64 <-> 8  if xlen == 64,
  "144" if xlen == 64 <-> 9  if xlen == 64,
  "160" if xlen == 64 <-> 10 if xlen == 64,
}

mapping rlist_stack_adj_base_rv32 : bits(4) <-> range(1, 4) = {
  0x4 <-> 1,
  0x5 <-> 1,
  0x6 <-> 1,
  0x7 <-> 1,
  0x8 <-> 2,
  0x9 <-> 2,
  0xa <-> 2,
  0xb <-> 2,
  0xc <-> 3,
  0xd <-> 3,
  0xe <-> 3,
  0xf <-> 4,
}

mapping rlist_stack_adj_base_rv64 : bits(4) <-> range(1, 7) = {
  0x4 <-> 1,
  0x5 <-> 1,
  0x6 <-> 2,
  0x7 <-> 2,
  0x8 <-> 3,
  0x9 <-> 3,
  0xa <-> 4,
  0xb <-> 4,
  0xc <-> 5,
  0xd <-> 5,
  0xe <-> 6,
  0xf <-> 7,
}

mapping rlist_stack_adj_base : bits(4) <-> stack_adj_base = {
  s if xlen == 32 <-> rlist_stack_adj_base_rv32(s) if xlen == 32,
  s if xlen == 64 <-> rlist_stack_adj_base_rv64(s) if xlen == 64,
}

mapping zcmp_assembly_mapping_stack_adj : (bits(4), stack_adj, bool) <-> string = {
  (rlist, stack_adj, is_negative) <-> "{" ^ spc() ^ "ra" ^ sep() ^ "{" ^ spc() ^ zcmp_assembly_rlist(rlist) ^ spc() ^ "}" ^ sep() ^ negative_sign(is_negative) ^ stack_adj_str(stack_adj),
}

mapping zcmp_assembly_mapping : (bits(4), bits(2), bool) <-> string = {
  forwards (rlist, spimm54, is_negative) => {
    let stack_adj = rlist_stack_adj_base(rlist) + unsigned(spimm54);
    zcmp_assembly_mapping_stack_adj(rlist, stack_adj, is_negative)
  },
  backwards str => {
    let (rlist, stack_adj, is_negative) = zcmp_assembly_mapping_stack_adj(str);
    let stack_adj_offset = stack_adj - rlist_stack_adj_base(rlist);
    let spimm54 = to_bits(2, stack_adj_offset);
    // to_bits is not well typed so double check this.
    assert(unsigned(spimm54) == stack_adj_offset);
    (rlist, spimm54, is_negative)
  },
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WOW

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.

3 participants