[PATCH] D68089: [InstCombine] Optimize some memccpy calls to memcpy/null
Dávid Bolvanský via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 29 12:46:09 PDT 2019
xbolva00 marked 5 inline comments as done.
xbolva00 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;
----------------
jdoerfert wrote:
> Unclear why we name Size.
Will changed.
================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1137
+ if (!N)
+ return nullptr;
+ size_t Pos = SrcStr.find(StopChar->getSExtValue());
----------------
jdoerfert wrote:
> Simplification nits: Move the 1136 check to 1130, then simplify 1131, and swap the cases in 1133.
Ok.
================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1138
+ return nullptr;
+ size_t Pos = SrcStr.find(StopChar->getSExtValue());
+ if (Pos == StringRef::npos) {
----------------
jdoerfert wrote:
> 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`?
Inspired by optimizeStrChr, that code uses Str.find(CharC->getSExtValue()); so I think it is OK.
================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1151
+ B.CreateMemCpy(Dst, 1, Src, 1, NewN);
+ return B.CreateInBoundsGEP(B.getInt8Ty(), Dst, NewN);
+}
----------------
jdoerfert wrote:
> 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()).
Ok.
================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1152
+ return B.CreateInBoundsGEP(B.getInt8Ty(), Dst, NewN);
+}
+
----------------
jdoerfert wrote:
> 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)
Yeah.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68089/new/
https://reviews.llvm.org/D68089
More information about the llvm-commits
mailing list