[PATCH] D53342: [SimplifyLibCalls] Mark known arguments with nonnull

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I added initial comments, will have to go through again though.



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:518
 
+  setNonNullParam(CI, 0);
   // See if we can get the length of the input string.
----------------
jdoerfert wrote:
> As above.
I'm confused, here and before, you annotate and later, under some condition, remove the annotation again. This seems risky.

Here for example, `Len = 0` will not remove the attribute but an unknown `Len` will. I doubt that is intentional.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:214
 
+static void annotateNonNull(CallInst *CI, ArrayRef<unsigned> ArgNos) {
+  Function *F = CI->getFunction();
----------------
Maybe make it clear that this annotates `nonnull` only if it is not a valid pointer, e.g., name this `annotateNonNullBasedOnAccess` or sth.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:217
+  if (!F)
+    return;
+
----------------
Please do not use this method, it is super confusing. I'd argue you should use the `getCallee` anyway but if you think the surrounding function is the one you need, I'd prefer `getCaller`.



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:225
+    if (!CI->paramHasAttr(ArgNo, Attribute::NonNull))
+      CI->addParamAttr(ArgNo, Attribute::NonNull);
+  }
----------------
If you do not keep track (=statistics), there is probably no need to check first, as below, I think.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:232
+    if (CI->paramHasAttr(ArgNo, Attribute::NonNull))
+      CI->removeParamAttr(ArgNo, Attribute::NonNull);
+  }
----------------
I don't think you need to check first.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:518
 
 Value *LibCallSimplifier::optimizeStpCpy(CallInst *CI, IRBuilder<> &B) {
   Function *Callee = CI->getCalledFunction();
----------------
What about this one?


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:678
+    return V;
+  annotateNonNull(CI, {0});
+  return nullptr;
----------------
I would argue we actually want to annotate `deref` all the time but do also do `nonnull` while we are at it. (The Attributor derives `nonnull` from `deref` if appropriate.) Long story short, why do we derive `nonnull` but not `deref(1)` here and in other places where we expect a `\0` char?

(We could also add `deref(N)` if we already computed the static string size.)


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:2381
+  else
+    removeNonNull(CI, {0});
+
----------------
We could even use "isKnownNonZero" or other helper functions here.


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

https://reviews.llvm.org/D53342