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

[Guillaume Chatelet via Phabricator via libc-commits]

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;
----------------
gchatelet wrote:
> 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.
The tests did catch it, I just didn't rerun them after changing this line...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150567