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 c.sext.w and zext.w #290

Merged

Conversation

AFOliveira
Copy link
Contributor

I'm now looking on to binutils and the ISA and trying to fix gaps between all the sources.

c.sext.w

zext.w

I also found that this repo doesn't have the any instruction with acquire and release, like "amoswap.b.rl", is there a reason for these, or can i also commit them?

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>
@@ -1 +1,3 @@
c.zext.w rd_rs1_p 1..0=1 15..13=4 12..10=7 6..5=3 4..2=4

$pseudo_op rv64_c::c.addiw c.sext.w rd_rs1_n0 15..13=1 12=0 6..2=0 1..0=1
Copy link
Member

Choose a reason for hiding this comment

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

c.addiw is part of the C extension, not Zcb, so this should be moved to rv64_c.

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 might be wrong here, but c.addiw is identified as part of the c extension by rv64_c::c.addiw and c.sext.w pseudo instruction appears to only be available in rv64_zcb. This is the case in the binutils and in the ISA manual

Copy link
Member

Choose a reason for hiding this comment

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

It looks like Binutils picks up sext.w as a pseudoinstruction for c.addiw when the C extension is enabled: https://github.com/bminor/binutils-gdb/blob/2754d75a11556577fcddf23718bfbff7d0489a3f/opcodes/riscv-opc.c#L580

But it only picks up c.sext.w as a pseudoinstruction for c.addiw when Zcb is enabled.

This all seems pretty illogical to me, but I guess we are stuck with the binutils behavior at this point for compatibility reasons, right @kito-cheng?

Copy link
Contributor Author

@AFOliveira AFOliveira Oct 4, 2024

Choose a reason for hiding this comment

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

Although it seems ilogical, my guess is binutils is doing that because the pseudo-instruction is only mentioned as a note on Zcb, maybe if the ISA clarified that, we could change all the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aswaterman Just a ping. Do we want to keep this on hold?
Technically, the PR is according to the rest of the sources. if anything changes in the meantime, I'll be happy to address those changes and PR any update.

@aswaterman aswaterman merged commit 8ae1c19 into riscv:master Oct 25, 2024
2 checks passed
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