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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2588
 Value *LibCallSimplifier::optimizeAtoi(CallInst *CI, IRBuilderBase &B) {
+  CI->addParamAttr(0, Attribute::NoCapture);
+
----------------
msebor wrote:
> 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.
Attribute queries on calls work by first checking whether the attribute exists on the call, and if it doesn't, checking whether it exists on the called function. If the called function has the attribute, then that implies that all calls have it as well.

The policy for libcall attributes is that all attributes that are unconditional are inferred on the declaration using inferNonMandatoryLibFuncAttrs(). This happens as part of the InferFunctionAttrs pass. Attributes that are conditional and cannot be inferred on the declaration, are placed by SimplifyLibCalls instead.

This is why the responsibility for inferring nocapture on atol falls to InferFunctionAttrs (it holds for all atol calls), while inferring nocapture on strtol falls to SimplifyLibCalls (it holds only for some strtol calls). If you run just InstCombine, it may look like nocapture doesn't get inferred, but it will get inferred in a complete optimization pipeline.

While inferring unconditional attributes in SimplifyLibCalls is fine from a correctness perspective, it is also unnecessary. We do not need (or want) to duplicate the functionality already present in inferNonMandatoryLibFuncAttrs() inside SimplifyLibCalls as well.


================
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);
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129224