[PATCH] D129224: [InstCombine] Fold strtoul and strtoull and avoid PR #56293

[Martin Sebor via Phabricator via llvm-commits]

msebor marked 2 inline comments as done.
msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:145
+  uint64_t Result = 0;
+  for (const char *Dig = Str.data(); *Dig; ++Dig) {
+    unsigned char DigVal = *Dig;
----------------
efriedma wrote:
> This assumes the string is null-terminated?  I mean, we don't have to guarantee any particular behavior if it isn't, but we shouldn't read past the end of the string.
Right.  I forgot that `getConstantStringInfo` actually returns an unterminated  array.  Thanks for pointing it out!  Let me fix that, add some tests, and update the comment above the function to make it clear.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:147
+    unsigned char DigVal = *Dig;
+    if (isdigit(DigVal))
+      DigVal = DigVal - '0';
----------------
efriedma wrote:
> Please use llvm::isDigit etc. The C library routines interact with the current locale.
Sure, I didn't notice the capitalization in existing code.

(My understanding was that the compiler sets the C locale but using the `llvm` routines makes sense to me, especially if there's any expectation to support EBCDIC in `-finput-charset=` at some point.)


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:164
+    unsigned long long PrevResult = Result;
+    Result = Result * Base + DigVal;
+
----------------
efriedma wrote:
> Maybe this would be easier to read using the SaturatingMultiplyAdd helper?
Sure.  (It requires making sure the first three arguments all have the same type, hence the further changes.)


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2591
 Value *LibCallSimplifier::optimizeAtoi(CallInst *CI, IRBuilderBase &B) {
+  CI->addParamAttr(0, Attribute::NoCapture);
+
----------------
xbolva00 wrote:
> This should go to BuildLibcalls as it is added unconditionally
Where in `BuildCalls` would you suggest to add it?

For what it's worth, `llvm::inferNonMandatoryLibFuncAttrs` already handles `LibFunc_atoi` but it's not invoked for calls to the function already in the source.  (I was under the impression is that `BuildCalls` is used when library call is being synthesized by the middle end, but I have very little experience in this area.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129224