[PATCH] D68089: [InstCombine] Optimize some memccpy calls to memcpy/null

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1126
+  ConstantInt *StopChar = dyn_cast<ConstantInt>(CI->getArgOperand(2));
+  ConstantInt *N = dyn_cast<ConstantInt>(Size);
+  StringRef SrcStr;
----------------
Unclear why we name Size.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1137
+  if (!N)
+    return nullptr;
+  size_t Pos = SrcStr.find(StopChar->getSExtValue());
----------------
Simplification nits: Move the 1136 check to 1130, then simplify 1131, and swap the cases in 1133.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1138
+    return nullptr;
+  size_t Pos = SrcStr.find(StopChar->getSExtValue());
+  if (Pos == StringRef::npos) {
----------------
I do not know so I ask, is SExt the right choice here or ZExt? StopChar could be `i8 -1` right? Now the question is, what do we want to find in SrcStr, `-1` or `255`?


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1151
+  B.CreateMemCpy(Dst, 1, Src, 1, NewN);
+  return B.CreateInBoundsGEP(B.getInt8Ty(), Dst, NewN);
+}
----------------
I guess it is always i8Ty but it would make it easier to read if we do not mix i8Ty and CI->getType(). (Just stick with the latter and CI->getType()->getPointerElementType()).


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1152
+  return B.CreateInBoundsGEP(B.getInt8Ty(), Dst, NewN);
+}
+
----------------
The return value is wrong if `std::min(Pos + 1, N->getZExtValue())` is not `Pos + 1` because then we should have return `null`. (I think)


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

https://reviews.llvm.org/D68089