[PATCH] D123629: [InstCombine] Fold memrchr calls with a constant character.

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:914
+      // to null regardless of Size.
+      return Constant::getNullValue(CI->getType());
+
----------------
nikic wrote:
> Use NullPtr?
Sure.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:922
+                                   "memrchr.ptr_plus");
+      return B.CreateSelect(Cmp, NullPtr, SrcPlus, "memrchr.sel");
+    }
----------------
nikic wrote:
> nikic wrote:
> > nikic wrote:
> > > I don't think this is correct. Consider an unknown size and a string with two occurrences of C in it. Now assume that at runtime, the size is chosen to lie between the two occurrences of C. I believe your implementation will return null for that case, while it should return the first occurrence.
> > Gah, I just realized that this code is only used for a known length, unlike the corresponding memchr() implementation. In this case EndOff will be the exact end offset, and not just an upper bound on it, and this code will always constant fold, so it's fine.
> > 
> > I think it would be clearer that this is a fully static if this directly emitted the appropriate result based on `LenC->getValue().ule(Pos)` rather than going through constant folding.
> Actually, if the length is constant, shouldn't this just directly return the GEP without any further comparisons?
You're right, thanks!  Your comment makes me realize it's possible to also fold the call when `n` isn't known as long as there's just one occurrence of the character in the string.  Let me do both.


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

https://reviews.llvm.org/D123629