[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:2588
 Value *LibCallSimplifier::optimizeAtoi(CallInst *CI, IRBuilderBase &B) {
+  CI->addParamAttr(0, Attribute::NoCapture);
+
----------------
nikic wrote:
> This call should still get dropped (the other one is fine, as it is conditional).
I'm not familiar with the guidelines for and against applying attributes to calls (or if there are any), but I added this one because I noticed that `nocapture` was missing from calls to `atol` while it was present in calls to `strtol`.  That seemed unexpected.

It sounds like you're suggesting that an attribute should only be applied to an argument of a call when it's not implied either by the explicit declaration of the called function in the IL, or by whatever might be implicitly added to it by the middle end.

I would find that subtle and surprising.  It would mean that an attribute could be used for codegen decisions even though it didn't actually appear anywhere in the IL.  From what I've seen, the IL is quite verbose and commonly redundant (e.g., the `noundef` and `nonnull` annotations added to calls to string function like `strlen` even when they are implied by accesses to declared objects that are readily apparent in the IL). 

So with that I prefer to leave this code in place for this change.  That said, in my testing not all calls with the same access semantics are annotated with the same attributes (e.g., a call to `atoi` is annotated differently than the corresponding call to `strlen`).  So I think there's room for improvement here and I'll try to look into it in a followup.


================
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:
> 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`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129224