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

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2603
+    // With a null EndPtr, this function won't capture the main argument.
+    // It would be readonly too, except that it still may write to errno.
+    CI->addParamAttr(0, Attribute::NoCapture);
----------------
nikic wrote:
> msebor wrote:
> > nikic wrote:
> > > That sounds like the parameter can still be readonly, just not the function.
> > I'm not sure I understand what you're suggesting here but the first argument is of course read-only regardless of whether or not the second one is null.  It seems that it could be annotated `Attribute::ReadOnly` unconditionally, as could many others in this file.  I just copied this bit from `LibCallSimplifier::optimizeStrTo` without thinking about what other attributes might be appropriate to add and under what conditions.  As there are no uses of `Attribute::ReadOnly` in this file I'm not sure that adding it just in this one case would be appropriate.  But as in the other case, I'd be happy to look in a followup into what other attributes these functions might benefit from.
> > 
> > (As an aside, annotating just the argument `readonly` doesn't have the effect I'd expect: calls to `strtol` are assumed to modify the object the first argument points to.  This is in contrast to calls to `atol` etc., where the whole function is annotated `readonly`.)
> I just checked, and the readonly attribute will already get added by InferFunctionAttrs, so there is nothing to do here.
> 
> > As an aside, annotating just the argument readonly doesn't have the effect I'd expect: calls to strtol are assumed to modify the object the first argument points to.
> 
> Do you have an example for this handy? I would expect this to work as long as the object is non-escaping. If it is potentially captured (say a function argument), then a non-argmemonly call might modify it through the capture. (LLVM doesn't model errno specially).
This might be easier to discuss outside of a review so I created PR #[[ https://github.com/llvm/llvm-project/issues/56744 | 56744 ]] with the details of both of the issues I was referring to here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129224