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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet marked 5 inline comments as done.
gchatelet added inline comments.


================
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:
> 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.
> 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.

`BcmpReturnType` is implicitly convertible from `uint32_t` by design. Since bool 
We could provide a `NONZERO` value though. The best value might be platform dependant.


================
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;
----------------
gchatelet wrote:
> sivachandra wrote:
> > 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.
> > 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.
> 
> `BcmpReturnType` is implicitly convertible from `uint32_t` by design. Since bool 
> We could provide a `NONZERO` value though. The best value might be platform dependant.
> Is this the correct check? Should we early-return on equality? IIUC, previously, it was doing an early return on XOR success.

The code is wrong indeed, I'm surprised that the tests didn't catch it. I'm investigating.


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

True, this might break on platforms where `unsigned` is different from `uint32_t`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150567