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 comments to helperrvv.h #527

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

sh1boot
Copy link
Contributor

@sh1boot sh1boot commented Mar 4, 2024

Depends on #520, and #521, #503, and #522.

The first commit is already in the #521 PR, but repeated here so I don't have to rebase #503 and #522.
The relevant changes are: rivosinc@78ea02c

@sh1boot sh1boot changed the title Dev/shosie/add rvv comments add comments to helperrvv.h Mar 4, 2024
@luhenry luhenry force-pushed the dev/shosie/add-rvv-comments branch from 78ea02c to f78a57b Compare March 7, 2024 17:48
@luhenry
Copy link
Contributor

luhenry commented Mar 7, 2024

@blapie rebased this one as well given all the previous PRs were merged.

@@ -1292,7 +1335,13 @@ static int vcast_i_vi2(vint2 v) {
//

static vquad loadu_vq_p(const int32_t *ptr) {
return SLEEF_RVV_DP_VREINTERPRET_VQ(SLEEF_RVV_DP_VREINTERPRET_4VU(SLEEF_RVV_SP_LOAD_2VI(ptr, VECTLENSP * 2)));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only removing a trailing space but doesn't change the code.

Copy link
Collaborator

@blapie blapie left a comment

Choose a reason for hiding this comment

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

Just pointing out a few typos. Otherwise lgtm

@@ -71,6 +71,11 @@
#endif

#if __riscv_v_intrinsic <= 12000
// __riscv_vcreate* intrinsics only showed up fairly recently, but are useful
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we give be a little bit more specific than "fairly recently"?

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've updated the comment but it turns out I'll need to update the logic as well.

It turns out it was only a coincidence that the feature added at the same time as the feature test macro changed in the compiler versions I checked. Clang took a double-step, updating to 0.12 and then progresively adding the draft features without a version bump. GCC is stuck in between, advertising 0.12 but not implementing vcreate (which is correct, I am wrong).

I'll submit a separate patch to test for clang-18 and up, as well as intrinsics version 1.0 and up.

src/arch/helperrvv.h Outdated Show resolved Hide resolved
src/arch/helperrvv.h Outdated Show resolved Hide resolved
@blapie
Copy link
Collaborator

blapie commented Mar 12, 2024

@luhenry @sh1boot I will take care of the general documentation on the website and README (as part of #524), but could you please provide a reference file like https://sleef.org/aarch64.xhtml for rvv types and functions.
Uploaded #529

@sh1boot sh1boot force-pushed the dev/shosie/add-rvv-comments branch from f78a57b to ad9a439 Compare March 12, 2024 17:56
@blapie blapie merged commit e9acd89 into shibatch:master Mar 13, 2024
31 checks passed
@sh1boot
Copy link
Contributor Author

sh1boot commented Mar 24, 2024

but could you please provide a reference file like https://sleef.org/aarch64.xhtml for rvv types and functions.

It's going to take me a while to find time for this (several weeks, at least). AIUI I can broadly copy-paste the SVE documentation but I'll also need to go through the list and make sure everything is present and true.

@luhenry luhenry deleted the dev/shosie/add-rvv-comments branch March 25, 2024 07:59
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.

None yet

3 participants