search

riscv-software-src / riscv-isa-sim

Spike, a RISC-V ISA Simulator
Other
2.38k stars 839 forks source link

Adding Zilsd and Zcmlsd extensions (Load/store pair for RV32) #1682

Closed christian-herber-nxp closed 3 months ago

christian-herber-nxp commented 4 months ago

Zilsd & Zcmlsd: https://github.com/riscv/riscv-zilsd

This is a PR the currently unratified load/store pair extension.

aswaterman commented 4 months ago

Can you squash these into a single commit?

I think this is ready for @jerryz123 to review.

jerryz123 commented 4 months ago

Why don't these instructions use the overlap_list mechanism?

aswaterman commented 4 months ago

I assume @christian-herber-nxp is following the pattern of e.g. https://github.com/riscv-software-src/riscv-isa-sim/blob/3a70f84b8a2249c92d35c2229b48ca5735a543fa/riscv/insns/c_fsw.h

But we probably should not proliferate that pattern. I agree overlap_list is cleaner and more self-documenting.

christian-herber-nxp commented 4 months ago

Can you squash these into a single commit?

I think this is ready for @jerryz123 to review.

I rebased and squashed. Well I see now that there are conflicts, so I need to revisit Edit: looks ok now.

christian-herber-nxp commented 4 months ago

I assume @christian-herber-nxp is following the pattern of e.g. https://github.com/riscv-software-src/riscv-isa-sim/blob/3a70f84b8a2249c92d35c2229b48ca5735a543fa/riscv/insns/c_fsw.h

But we probably should not proliferate that pattern. I agree overlap_list is cleaner and more self-documenting.

Correct. I do not have experience with Spike so I just tried to adopt what is there. Had a look at the overlap_list. As this is based on an unordered set, it does sound like a slight overkill to me. Did you ever check the performance compared to a small if-else? Of course, it is your choice how you would like to have this integrated. As this would also affect the existing c.flw/c.ld overlaps etc., maybe this goes beyond the scope of this PR.

aswaterman commented 4 months ago

The performance is actually neutral: neither the overlap list nor the if-statement has a runtime performance impact. (The overlap list is only used when setting up the simulation, not when running; and the if-statement is compiled out.) So it's more about aesthetics.

Let me prepare a PR for the existing C instructions (like c.flw/c.ld) and see if we like it.

aswaterman commented 3 months ago

@christian-herber-nxp Now that #1687 has been merged, I think you can proceed with reworking this to move the code into c_ld[sp].h and c_sd[sp].h.

In this case, you shouldn't add anything to the overlap list, I think.

aswaterman commented 3 months ago

Manually merged via 70d26d64e6ba2da329357a88dc313277fff6c22c