[PATCH] D122686: [InstCombine] Fold calls to strnlen (PR46455)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

It would be good to split this up into smaller parts where possible. Two obvious bits would be to precommit the tests with baseline checks (NFC) and land the addition of nonnull attributes on strnlen (which is trivial).



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:644
+      return B.CreateBinaryIntrinsic(Intrinsic::umin, Min, Bound);
+    };
+  }
----------------
It would be fine to just emit the umin unconditionally here and let it fold away if it happens to be constant, something like:
```
Value *Min = ConstantInt::get(CI->getType(), Len - 1);
return [&]() {
  return Bound ? B.CreateBinaryIntrinsic(Intrinsic::umin, Min, Bound) : Min;
};
```


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:672
         }
-        // If the string does not have '\0', leave it to strlen to compute
-        // its length.
-        if (NullTermIdx == ~((uint64_t)0))
+        if (NullTermIdx == ~((uint64_t)0) && MaxLen > Slice.Length)
+          // If the array does not have '\0', leave it to either strlen
----------------
I had a bit of a hard time understanding this new special case. I'm wondering, if you have no null and use strnlen, can't we simply always return `Bound` as the result? Either the result will be Bound, or we're reading past the extent of the global and have UB. Or if we don't want to optimize the latter case we can keep the MaxLen < ArraySize check, but in either case I think mixing it with the code below is kind of confusing -- in particular the `sub` bit doesn't seem relevant?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:710
+          // and let the caller simplify that better.
+          return nullptr;
+
----------------
If we moved handling of the isOnlyUsedInZeroEqualityComparison() check in optimizeStringLength() before the minStringLength() handling, could we drop the special check here?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:768
     }
   }
 
----------------
Might be more elegant with `m_ConstantInt`, something like...
```
uint64_t MaxLen;
if (!Bound || !match(Bound, m_ConstantInt(MaxLen))
  MaxLen = UINT64_MAX;

if (MaxLen == 0)
  return ConstantInt::get(CI->getType(), 0);
```


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:777
+  if (!isOnlyUsedInZeroEqualityComparison(CI) ||
+      (Bound && !isKnownNonZero(Bound, DL) && MaxLen == UINT64_MAX))
+    // Avoid folding strnlen(x, N) == 0 unless N is known to be nonzero.
----------------
Why does this need the `MaxLen == UINT64_MAX` check?


================
Comment at: llvm/test/Transforms/InstCombine/strlen-4.ll:4
+;
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
----------------
Please use llvm/utils/update_test_checks.py to generate InstCombine test checks.


================
Comment at: llvm/test/Transforms/InstCombine/strnlen-5.ll:2
+; Verify that equality tests of strnlen calls against zero are folded
+; correctly.
+;
----------------
This test file could probably be called `strnlen-icmp.ll` or something? Just numbering them isn't super meaningful.


================
Comment at: llvm/test/Transforms/InstCombine/strnlen-6.ll:53
+; CHECK-NEXT:    %ptr = load i8*, i8** @ecp
+; CHECK-NEXT     %len = call i64 @strnlen(i8* oundef nonnull dereferenceable(1) %ptr, i64 3)
+; CHECK-NEXT     ret i64 %len
----------------
This test output looks wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122686