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

Added some V extension Pseudo-instructions #295

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

Conversation

AFOliveira
Copy link
Contributor

Added some V extension Pseudo-instructions described on the ISA Manual and implemented on binutils. I'm working on adding the rest of them.

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>
@aamartin0000
Copy link

Shouldn't the "gt" pseudos alias to "le", not "lt" ?

@AFOliveira
Copy link
Contributor Author

@aamartin0000 I believe you're confusing this with negation, in which case it woukd be correct. However, if you take a closer look from an hardware standpoint, just switching the arguments wouldn't make it a negation since if they are equal(or the difference between them is 0), the branch is supposed to happen either way,

@aamartin0000
Copy link

@aamartin0000 I believe you're confusing this with negation, in which case it woukd be correct. However, if you take a closer look from an hardware standpoint, just switching the arguments wouldn't make it a negation since if they are equal(or the difference between them is 0), the branch is supposed to happen either way,

Ok, I get it. If the two operands have the same value, flt will be false, and so will fgt, while fge would be true (unless the value is NaN).

Next question: you have
$pseudo_op rv_v::vmflt.vf vmfgt.vv 31..26=0x1b vm rs1 vs2 14..12=0x5 vd 6..0=0x57
$pseudo_op rv_v::vmfle.vf vmfge.vv 31..26=0x19 vm vs2 rs1 14..12=0x5 vd 6..0=0x57

This is somewhat confusing because you are aliasing the .vv form with a .vf op, with rs1 instead of vs1. Should these not be using vmflt.vv/vmfle.vv ?

rv_v Outdated
@@ -138,6 +138,11 @@ vfwnmacc.vf 31..26=0x3d vm vs2 rs1 14..12=0x5 vd 6..0=0x57
vfwmsac.vf 31..26=0x3e vm vs2 rs1 14..12=0x5 vd 6..0=0x57
vfwnmsac.vf 31..26=0x3f vm vs2 rs1 14..12=0x5 vd 6..0=0x57

#Pseudo OPFVF
$pseudo_op rv_v::vmflt.vf vmfgt.vv 31..26=0x1b vm rs1 vs2 14..12=0x5 vd 6..0=0x57
$pseudo_op rv_v::vmfle.vf vmfge.vv 31..26=0x19 vm vs2 rs1 14..12=0x5 vd 6..0=0x57
Copy link
Member

Choose a reason for hiding this comment

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

How can a vv variant be a pseudo of a vf variant? I assume what you want here is something like

$pseudo_op rv_v::vmflt.vv    vmfgt.vv 31..26=0x1b vm vs1 vs2 14..12=0x5 vd 6..0=0x57
$pseudo_op rv_v::vmfle.vv    vmfge.vv 31..26=0x19 vm vs1 vs2 14..12=0x5 vd 6..0=0x57

but I don't see how this communicates to the downstream tooling that vs1 and vs2 are swapped. Are we missing some additional functionality that we need to implement before downstream tools can make use of these swapped-argument pseudos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This are wrong, sure, they should be vv, my bad. I'm not quite sure yet how they are parsed differently, I'll take a look and answer back on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instruction descriptionts are fixed. I'll wait for #283 to do the rest of the required work.

rv_v Show resolved Hide resolved
rv_v Show resolved Hide resolved
$pseudo_op rv_v::vmslt.vv vmsgt.vv 31..26=0x1b vm vs1 vs2 14..12=0x0 vd 6..0=0x57
$pseudo_op rv_v::vmsltu.vv vmsgtu.vv 31..26=0x1a vm vs1 vs2 14..12=0x0 vd 6..0=0x57
$pseudo_op rv_v::vmsle.vv vmsge.vv 31..26=0x1d vm vs1 vs2 14..12=0x0 vd 6..0=0x57
$pseudo_op rv_v::vmsleu.vv vmsgeu.vv 31..26=0x1c vm vs1 vs2 14..12=0x0 vd 6..0=0x57
Copy link
Member

Choose a reason for hiding this comment

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

Same concern as for the FP comparisons.

@@ -411,6 +427,13 @@ vwmaccu.vv 31..26=0x3c vm vs2 vs1 14..12=0x2 vd 6..0=0x57
vwmacc.vv 31..26=0x3d vm vs2 vs1 14..12=0x2 vd 6..0=0x57
vwmaccsu.vv 31..26=0x3f vm vs2 vs1 14..12=0x2 vd 6..0=0x57

#Pseudo-Instructions for Vector Integer Extension Instructions

$pseudo_op rv_v::vmxor.mm vmclr.m 31..26=0x1b 25=1 vs2=vd vs1=vd 14..12=0x2 vd 6..0=0x57
Copy link
Member

@aswaterman aswaterman Oct 7, 2024

Choose a reason for hiding this comment

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

More of a question than a comment: is there a reason for using vs2=vd rather than vs2=vs1 here? By transitivity, they're equivalent; I'm just curious if there should be a stylistic default and, if so, why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to ensure that the parsing assumes them as the same value. This is extremely useful when generating, per example, the yaml_dump since we can know what differs from the original instruction to the pseudo-instruction.

This can be parsed to ouput, per example, the following: https://github.com/riscv-software-src/riscv-unified-db/blob/00213d2b2703240dc4fbe90dcdd8749e5b5f7e35/arch/inst/B/add.uw.yaml#L25

Copy link
Member

Choose a reason for hiding this comment

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

I think you missed my point. There are two equivalent ways of expressing this: we can say vs2=vd and vs1=vd, or we can say vs1=vd and vs2=vs1. The same information would be conveyed to parsing tools either way. I am just wondering why we are anchoring on vd.

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 definitely missed it, but I believe the best way to go for it is to say that "original_field = other_field", since parsing wise this is more helpful

rv_v Show resolved Hide resolved
Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

placing a hold on this PR until the swapped-argument pseudo issue is resolved.

@AFOliveira
Copy link
Contributor Author

placing a hold on this PR until the swapped-argument pseudo issue is resolved.

I really apologize for my delay on this, but I was really busy to pick up this changes on the parsing and still ensure they would be 100% correct, I'll be looking into them now. Though I should probably wait for #283 before doing any changes, right?

@aswaterman
Copy link
Member

I really apologize for my delay

No problem!

Though I should probably wait for #283 before doing any changes, right?

Yeah, probably so.

Signed-off-by: Afonso Oliveira <Afonso.Oliveira@synopsys.com>
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