[PATCH] D128364: [InstCombine] Look through more casts when folding memchr and memcmp

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4196
+    Op0 = Op0->stripPointerCasts();
     const GlobalVariable *GV = dyn_cast<GlobalVariable>(Op0);
     if (!GV)
----------------
efriedma wrote:
> Maybe it makes sense to just drop the `dyn_cast<GlobalVariable>` here?
This is what I meant by the simplification opportunity (I think we discussed it with @nikic in another review but I should have made that clear here).  As far as I can tell the `GlobalVariable` cast is necessary to get at the `DataLayout` and to compute the offset below.  (Or is there some other way to get at it?)


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4287
   if (Slice.Array == nullptr) {
-    if (TrimAtNul) {
+    if (Slice.Length && TrimAtNul) {
       Str = StringRef();
----------------
efriedma wrote:
> Not sure I understand this change; why does it matter if the slice is zero-length here?
The change prevents treating past-the-end accesses to `char` arrays the same was as those to empty strings, such as in `strlen("123" + 4)`.  This is exercised by `str-int-3.ll` that I added in D125114 based on my understanding of the convention that library calls that provably result in an out-of-bounds access are best left for sanitizers to report.  (I think an argument can be made that folding such calls to zero is safer than counting on sanitizers, so I don't mind removing the check and adjusting the test if it's decided that's preferable.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128364