[PATCH] D122836: Fold more memchr calls

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I've committed your tests with baseline checks in https://github.com/llvm/llvm-project/commit/371d2ed3f3d7f67602ff9956510f3e2d9df3d5b0, can you please rebase on top of that?



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:907
+    CharVal = B.CreateTrunc(CharVal, B.getInt8Ty());
+    CharVal = B.CreateZExt(CharVal, Val->getType());
+    Value *Cmp = B.CreateICmpEQ(Val, CharVal, "memchr.char0cmp");
----------------
Why does this zext Val and CharVal? Isn't the trunc of CharVal to i8 and a comparison in i8 type sufficient?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:925
+    if (MaxLen == UINT64_MAX) {
+      // Fold memchr(s, c, n) -> n < Pos ? null : s + Pos
+      Value *Cmp = B.CreateICmpULE(Size, ConstantInt::get(Size->getType(), Pos),
----------------
Should be `n <= Pos` in the comment to match the code.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:934
+
+    if (MaxLen < Pos)
+      // When the constant Size is less than the character position
----------------
Should be `MaxLen <= Pos`?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:941
+    // to a pointer to the array or to null.
+    MaxLen = Pos < MaxLen ? Pos + 1 : Pos;
+  } else if (MaxLen == UINT64_MAX)
----------------
Not sure why we need to fall through to the code below here -- or maybe more generally, can we just drop the `MaxLen == UINT64_MAX` condition above and always emit the icmp+select sequence? Will get folded for the case where the size is known.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1003
   // Check if all arguments are constants.  If so, we can constant fold.
   if (!CharC)
     return nullptr;
----------------
Is the code below here rendered redundant by your new code above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122836