[libc-commits] [PATCH] D82134: [libc] Add strcmp implementation.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/strcmp.cpp:15
+
+int LLVM_LIBC_ENTRYPOINT(strcmp)(const char *l, const char *r) {
+  while (*l) {
----------------
gchatelet wrote:
> cgyurgyik wrote:
> > PaulkaToast wrote:
> > > sivachandra wrote:
> > > > Is there benefit in comparing words at a time? I know that it leads to potential OOB reads, so we can handle it separately. But if it can lead to speed up, just a leave a TODO about it and we will get to it in a later round.
> > > I would suggest to opt for more descriptive variable names over single letter one. i.e ("left" instead of "l").
> > What is the standard for leaving TODOs? I'm not finding anything in the llvm Coding Standards.
> > Is there benefit in comparing words at a time? I know that it leads to potential OOB reads, so we can handle it separately. But if it can lead to speed up, just a leave a TODO about it and we will get to it in a later round.
> 
> Technically we have to read the buffers byte per byte as we're not supposed to read past the `\0`.
> I'm not optimistic on the ability to accelerate this routine because -even if a page fault can only occur at page boundaries- some processors are offering [[ https://developer.arm.com/-/media/Arm%20Developer%20Community/PDF/Learn%20the%20Architecture/Providing%20protection%20for%20complex%20software.pdf | hardware pointer authentication ]] which can break reading buffers cache line per cache line.
> > Is there benefit in comparing words at a time? I know that it leads to potential OOB reads, so we can handle it separately. But if it can lead to speed up, just a leave a TODO about it and we will get to it in a later round.
> 
> Technically we have to read the buffers byte per byte as we're not supposed to read past the `\0`.
> I'm not optimistic on the ability to accelerate this routine because -even if a page fault can only occur at page boundaries- some processors are offering [[ https://developer.arm.com/-/media/Arm%20Developer%20Community/PDF/Learn%20the%20Architecture/Providing%20protection%20for%20complex%20software.pdf | hardware pointer authentication ]] which can break reading buffers cache line per cache line.

So, going by that, we cannot employ similar techniques (of comparing word lengths at a time) even for functions like `strlen`. Just to note, few other libcs take such an approach with `strlen`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82134