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

[Dávid Bolvanský via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:518
 
+  setNonNullParam(CI, 0);
   // See if we can get the length of the input string.
----------------
jdoerfert wrote:
> 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.
Len = 0 means we return Dst;

I need to reorder the code here a bit..


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:678
+    return V;
+  annotateNonNull(CI, {0});
+  return nullptr;
----------------
jdoerfert wrote:
> 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.)
deref(N) for string constants? 
- useless, no?

And, isn't it a attributor's work? get GEP with bound N and infer  deref(N)

-------
I agree with deref(1).


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

https://reviews.llvm.org/D53342