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

Naming of cv.slet #833

Closed
realqhc opened this issue Jul 14, 2023 · 3 comments
Closed

Naming of cv.slet #833

realqhc opened this issue Jul 14, 2023 · 3 comments
Assignees
Labels
Component:Doc For issues in the Documentation (e.g. for README.md files) Type:Enhancement For feature requests and enhancements

Comments

@realqhc
Copy link

realqhc commented Jul 14, 2023

On a review for upstreaming core-v alu encoding, the reviewer Craig Topper asked

I would have expected cv.sle and cv.sleu for these mnemonics. The t feels out of place. In the base isa for slt/sltu it's part of "less than". Is the instruction name here "set less equal than" instead "set less than or equal"?

The vector spec uses "sle" and "sleu"

Is the spec frozen?

Is it possible that anyone could provide a clarification on the meaning of the mnemonics cv.slet, cv.sletu?

@pascalgouedo pascalgouedo added the Component:Doc For issues in the Documentation (e.g. for README.md files) label Jul 17, 2023
@pascalgouedo pascalgouedo self-assigned this Jul 17, 2023
@pascalgouedo pascalgouedo added the Type:Enhancement For feature requests and enhancements label Jul 17, 2023
@pascalgouedo
Copy link

pascalgouedo commented Jul 17, 2023

Hi @realqhc

Those instructions are executing a "set if less than or equal" operation.
So I agree that cv.sle/cv.sleu would be a more standard way of mnemonic naming.
It has been inherited from PULP RI5CY and I didn't figured out this (mnemonic) point before.

As PULP instructions are close to frozen (especially in GNU toolchain), I will ask to all concerned people if renaming is still possible at this point of the project.

Thank you anyway for your comment/issue.

Best regards,
Pascal.

@davideschiavone
Copy link
Contributor

@realqhc , can we close this issue?

pascalgouedo pushed a commit to pascalgouedo/cv32e40p that referenced this issue Aug 4, 2023
Signed-off-by: Pascal Gouedo <[email protected]>
@pascalgouedo
Copy link

Only once #844 merged ...

@realqhc realqhc closed this as completed Aug 4, 2023
topperc added a commit to topperc/llvm-project that referenced this issue Sep 13, 2024
According to openhwgroup/cv32e40p#833
this instruction was renamed last year to remove the 't'.

I used MnemonicAlias to support the old name. Unfortunately, this
gives a generic error if XCValu is not enabled. Since its an old
name I hope this isn't too big of an issue.
topperc added a commit to llvm/llvm-project that referenced this issue Sep 13, 2024
According to openhwgroup/cv32e40p#833 this
instruction was renamed last year to remove the 't'.

I used MnemonicAlias to support the old name. Unfortunately, this gives
a generic error if XCValu is not enabled. Since its an old name I hope
this isn't too big of an issue.

CC: @jeremybennett
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:Doc For issues in the Documentation (e.g. for README.md files) Type:Enhancement For feature requests and enhancements
Projects
None yet
Development

No branches or pull requests

3 participants