[libc-commits] [PATCH] D150567: [libc] Add optimized bcmp for RISCV

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/memory_utils/bcmp_implementations.h:27
   LIBC_LOOP_NOUNROLL
-  for (size_t offset = 0; offset < count; ++offset)
-    if (auto value = generic::Bcmp<1>::block(p1 + offset, p2 + offset))
----------------
What was `offset` previously? Was this dead code?


================
Comment at: libc/src/string/memory_utils/bcmp_implementations.h:28
+  for (; offset < count; ++offset)
+    if (uint32_t value = (p1[offset] == p2[offset]))
       return value;
----------------
s/`uint32_t`/`BcmpReturnType` ? I think it might not compile? May be a cleaner approach would be to provide a `BcmpReturnType::TRUE` and `BcmpReturnType::FALSE`, or may be `BmpReturnType::ONE`.


================
Comment at: libc/src/string/memory_utils/bcmp_implementations.h:28
+  for (; offset < count; ++offset)
+    if (uint32_t value = (p1[offset] == p2[offset]))
       return value;
----------------
sivachandra wrote:
> s/`uint32_t`/`BcmpReturnType` ? I think it might not compile? May be a cleaner approach would be to provide a `BcmpReturnType::TRUE` and `BcmpReturnType::FALSE`, or may be `BmpReturnType::ONE`.
Is this the correct check? Should we early-return on equality? IIUC, previously, it was doing an early return on XOR success.


================
Comment at: libc/src/string/memory_utils/bcmp_implementations.h:56
+    if (a != b)
+      return 1U;
+  }
----------------
Same here: `1U` need not always be of `uint32_t` type.


================
Comment at: libc/src/string/memory_utils/bcmp_implementations.h:81
+    if (a != b)
+      return 1U;
+  }
----------------
And here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150567/new/

https://reviews.llvm.org/D150567