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

Fix RVV implementation #410

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Fix RVV implementation #410

merged 4 commits into from
Apr 22, 2024

Conversation

WojciechMula
Copy link
Collaborator

We have problems with UTF-16 to UTF-32. This is my attempt to fix it.

@WojciechMula
Copy link
Collaborator Author

A bit of explanation: I followed the AVX-512 way pretty closely. We load VL elements, but process VL-1 pairs of u16 words. RVV does not allow shifting masks(*) as AVX512 does, so there are a few scalar checks.

    • in a portable way

Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

Looks good. Merge at will.

Assuming that the issue is fixed, we'll issue a release soon after.

@WojciechMula
Copy link
Collaborator Author

It would be perfect if @camel-cdr tested this on real hardware.

@lemire
Copy link
Member

lemire commented Apr 17, 2024

@camel-cdr Can you test it out?

@camel-cdr
Copy link
Contributor

@lemire @WojciechMula

I tried running it on the kendryte k230, and it seems to work, except for returning the correct error index and basic_fuzzer timeout:

67/76 Test #67: convert_utf16le_to_utf32_with_errors_tests ....***Failed    0.32 sec
Checking implementation rvv
Running 'convert 2 UTF16 bytes'... .......... OK
Running 'convert with surrogates'... .......... OK
Running 'convert fails if there is sole low surrogate'... lhs: res.count = 0
rhs: i = 1
res.count
file /root/t/simdutf/tests/convert_utf16le_to_utf32_with_errors_tests.cpp:73, function operator()

      Start 68: convert_utf16be_to_utf32_with_errors_tests
68/76 Test #68: convert_utf16be_to_utf32_with_errors_tests ....***Failed    0.32 sec
Checking implementation rvv
Running 'convert 2 UTF16 bytes'... .......... OK
Running 'convert with surrogates'... .......... OK
Running 'convert fails if there is sole low surrogate'... lhs: res.count = 0
rhs: i = 1
res.count
file /root/t/simdutf/tests/convert_utf16be_to_utf32_with_errors_tests.cpp:79, function operator()

      Start 69: convert_valid_utf16le_to_utf32_tests

         67 - convert_utf16le_to_utf32_with_errors_tests (Failed)
         68 - convert_utf16be_to_utf32_with_errors_tests (Failed)
         75 - basic_fuzzer (Timeout)

The following patch seems to fix the index error, I'm not completely sure why the +1 is needed though.

--- a/src/rvv/rvv_utf16_to.inl.cpp
+++ b/src/rvv/rvv_utf16_to.inl.cpp
@@ -256,9 +256,9 @@ simdutf_really_inline static result rvv_utf16_to_utf32_with_errors(const char16_

     {
         const vbool8_t diff = __riscv_vmxor_mm_b8(surhi, surlo, vl);
-        if (__riscv_vfirst_m_b8(diff, vl) >= 0) {
-          const size_t idx = 0; // XXX: calculate it properly
-          return result(error_code::SURROGATE, src - srcBeg + idx);
+       const long idx = __riscv_vfirst_m_b8(diff, vl);
+        if (idx >= 0) {
+          return result(error_code::SURROGATE, src - srcBeg + idx + 1);
         }
     }

The basic_fuzzer timed out, and I'm currently running it standalone, but it has been at 'basic fuzz' for more than an hour now.
I'm not sure if it encounters an infinite loop or is just really slow on my hardware.
The only changes to the test code since it worked are debug prints, so this must be caused by a rvv code change.
I don't see how the code from this PR could cause an infinite loop though, it always decrements len, or errors.

I'll try to investigate with gdb.

@camel-cdr
Copy link
Contributor

Ahh, I get it. The infinite loop is cause by the

vl = __riscv_vsetvl_e16m2(vl - 1);

when that branch is taken and vl=1.
This should be an easy fix by just erroring when vl=0, so a single character is supposed to contain a surrogate pair:

--- a/src/rvv/rvv_utf16_to.inl.cpp
+++ b/src/rvv/rvv_utf16_to.inl.cpp
@@ -247,6 +247,8 @@ simdutf_really_inline static result rvv_utf16_to_utf32_with_errors(const char16_
     // decode surrogates
     vuint16m2_t v1 = __riscv_vslide1down_vx_u16m2(v0, 0, vl);
     vl = __riscv_vsetvl_e16m2(vl - 1);
+    if (vl == 0)
+          return result(error_code::SURROGATE, src - srcBeg);

     const vbool8_t surhi  = __riscv_vmseq_vx_u16m2_b8(__riscv_vand_vx_u16m2(v0, HI_SURROGATE_MASK, vl), HI_SURROGATE_VALUE, vl);
     const vbool8_t surlo  = __riscv_vmseq_vx_u16m2_b8(__riscv_vand_vx_u16m2(v1, LO_SURROGATE_MASK, vl), LO_SURROGATE_VALUE, vl);

@WojciechMula maybe there is a more idiomatic way to fix this, but the above seems to work.

This completes the basic_fuzzer test, and I'm rerunning all other tests now.

@WojciechMula
Copy link
Collaborator Author

@camel-cdr Thanks a lot for your effort! I'll try to apply the fixes tomorrow, this week is really weird for me.

Co-authored-by: Camel Coder <camel-cdr@protonmail.com>
@WojciechMula
Copy link
Collaborator Author

@camel-cdr applied your fixes, everything works. (Except a few tests that fail on my machine due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114747)

@lemire
Copy link
Member

lemire commented Apr 22, 2024

Let us merge and release.

@lemire lemire merged commit 1bc60f0 into simdutf:master Apr 22, 2024
45 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.

None yet

3 participants