[PATCH] D122836: [SimplifyLibCalls] Fold more memchr calls

[Martin Sebor via Phabricator via llvm-commits]

msebor added a comment.

Thanks again for committing the baseline tests for me and (to both of you) for your comments.  Let me upload a revised patch.



================
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");
----------------
nikic wrote:
> Why does this zext Val and CharVal? Isn't the trunc of CharVal to i8 and a comparison in i8 type sufficient?
I believe it is.  It just needs an adjustment to the existing `memchr.ll` test.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:924
+
+    if (MaxLen == UINT64_MAX) {
+      // Fold memchr(s, c, n) -> n < Pos ? null : s + Pos
----------------
xbolva00 wrote:
> !LenC ?
Sure.


================
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),
----------------
nikic wrote:
> Should be `n <= Pos` in the comment to match the code.
Done.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:934
+
+    if (MaxLen < Pos)
+      // When the constant Size is less than the character position
----------------
nikic wrote:
> Should be `MaxLen <= Pos`?
Either works thanks to what follows but sure.


================
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)
----------------
nikic wrote:
> 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.
Good idea!  It simplifies the rest of the function as well.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:942
+    MaxLen = Pos < MaxLen ? Pos + 1 : Pos;
+  } else if (MaxLen == UINT64_MAX)
+    // From now on we need at least constant length and constant array.
----------------
xbolva00 wrote:
> If possible better to check for LenC than comparing with UINT64_MAX.. 
Done.


================
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;
----------------
nikic wrote:
> Is the code below here rendered redundant by your new code above?
Yes, I have removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122836