[PATCH] D123819: [InstCombine] Fold strlen and strnlen recursively.

[Martin Sebor via Phabricator via llvm-commits]

msebor added a comment.

In D123819#3487880 <https://reviews.llvm.org/D123819#3487880>, @nikic wrote:

> As mentioned on the original review, I think we would be better off not passing the Bound to minStringLength() and instead doing one umin at the end -- this avoids the need to handle this in each branch. Though possibly I forgot some reason why that doesn't work from the previous discussion.

You made this suggestion in the context of the select statement where I implemented it (see line 748).  Removing the `Bound` (and `MaxLen`) from the function completely would prevent handling variable offsets in `strlen` while punting on them in `strnlen` as you requested in D122686 <https://reviews.llvm.org/D122686>.  It would also prevent ultimately handling it in `strnlen` as proposed D123820 <https://reviews.llvm.org/D123820> in cases that the `strlen` folding cannot handle (arrays with embedded nuls where the bound is less than the length of any of the substrings).

Splitting up the variable offset handling between two patches makes the work harder to think about and, it seems, also harder to review, rather than easier as you'd hoped.  Would it help if I merged them into one?



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:635
+LibCallSimplifier::CreateInsnFuncTy
+LibCallSimplifier::minStringLength(CallInst *CI, Value *Src, IRBuilderBase *B,
+                                   unsigned CharSize, uint64_t MaxLen,
----------------
nikic wrote:
> Can pass in IRBuilderBase by-reference? It seems like part of the changes here are just `B.` to `B->`.
Passing it by reference means the lambda capture cannot be simply by value.  Instead, each lambda must explicitly specify the capture for each variable, making it more verbose, harder to read, etc.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:714
           (isa<GlobalVariable>(GEP->getOperand(0)) &&
-           NullTermIdx == ArrSize - 1)) {
-        Offset = B.CreateSExtOrTrunc(Offset, CI->getType());
-        return B.CreateSub(ConstantInt::get(CI->getType(), NullTermIdx),
-                           Offset);
+           (NullTermIdx == ArrSize - 1) && (!Bound || MaxLen < UINT64_MAX))) {
+        // Fold strnlen(s + x, Bound) -> min(strlen(s) - x, Bound)
----------------
nikic wrote:
> Why is this condition needed?
To prevent folding `strnlen` calls with variable offsets as you requested in the previous review.  See for example `fold_strnlen_s3_pi_n` in `strnlen-3.ll` and others.  The folding is enabled in D123820.


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

https://reviews.llvm.org/D123819