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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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.
----------------
xbolva00 wrote:
> 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..
Oh, this is all confusing (as I said).


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:217
+  if (!F)
+    return;
+
----------------
xbolva00 wrote:
> jdoerfert wrote:
> > 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`.
> > 
> Ok, getCaller.
> 
> CallInst has no getCallee.
`getCallee -> getCalledFunction` 

I still think the callee is what is important here (conceptually), though I doubt it makes a difference in practice.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:678
+    return V;
+  annotateNonNull(CI, {0});
+  return nullptr;
----------------
xbolva00 wrote:
> 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).
> deref(N) for string constants?
Yes, or more general, for everything we know a minimal "length".
> Useless?
I don't think so, but see below.
> Strictly needed?
No. Though, we do already call `GetStringLength` all the time here, why not manifest the information?
> And, isn't it a attributor's work? get GEP with bound N and infer deref(N)
It does, yes.

> `nonnull -> deref(1)`
That doesn't happen in the Attributor right now but it should because `deref(1)` can be combined (e.g., in a phi) with `null` to `deref_or_null(1)` while `nonnull` cannot be combined with `null` to anything.


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

https://reviews.llvm.org/D53342