-
Notifications
You must be signed in to change notification settings - Fork 7
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
assembler: Add XTheadCondMov support #16
Conversation
Sorry about the delay reviewing this, it's been a little all over the place. Will get around to looking at this and (primarily) the spec docs tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit, but this looks fine overall
@@ -0,0 +1,33 @@ | |||
#include <catch/catch.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps rename this file to assembler_xthead_tests.cpp
so it makes adding further xthead tests a little more straightforward without needing to add more cpp files (mainly because I'll add support for the rest of the Xthead instructions in the assembler, so it saves me a little busywork).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the extensions in that spec are not useful (at least to me) in the presence of other official extensions, and they may even be breaking stuff.
For example, XTheadVector is a "non-standard non-conforming extension", I find no reason to use it when there's V, since (afaics) they don't interact with each other, meaning you can't use those instructions on the standard vector registers.
XTheadCondMov is useful to me because (imo) they sorta messed up with Zicond.
On the other hand, some other extensions might be useful even if there's better standard alternatives.
For example XTheadBa which defines ADDSL, which works like SH1/2/3ADD - but might be useful for chips that have XTheadBa but not Zba.
Thought I'd let you know, I'll do the rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, looks good!
Useful extension for implementing conditional moves in a manner more similar to x86/ARM.
The standard Zicond way to do conditional movs is czero.nez + czero.eqz + or/add, while XTheadCondMov can do it in 1 instruction.
For example:
versus
GCC picks it over Zicond when both are available: https://godbolt.org/z/bfG7reh8W
Documentation is in here: https://github.com/XUANTIE-RV/thead-extension-spec/releases/tag/2.3.0