[PATCH] D27855: [InstCombine] try to extend nonnull-ness of arguments from a callsite back to its parent function

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D27855#625390, @spatel wrote:

> In https://reviews.llvm.org/D27855#625386, @mehdi_amini wrote:
>
> > Isn't this something that could be part of the "Deduce function attributes" pass?
>
>
> I actually looked at that first, but this (nonnull) is a parameter attribute rather than a function attribute, so it didn't seem like the right place. If I misunderstood the boundary, please let me know.


This time I looked closer, and sure enough we have FunctionAttrs.cpp::addArgumentAttrs(). So there already exists a place where we could splice this in. However, it still seems mismatched in intent because that function is dealing with interprocedural transforms, while this patch is limited to analysis within a single function. If anyone thinks it's better handled there, I'm not opposed, but I haven't thought of a compelling reason for that yet. It seems logical to me to keep this next to the existing InstCombine nonnull transform since this transform can enable that one.



================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2779-2820
+/// If a callsite has arguments that are also arguments to the parent function,
+/// try to propagate attributes from the callsite's arguments to the parent's
+/// arguments.
+static Instruction *extendArgAttrsToCaller(CallSite &CS, DominatorTree &DT) {
+  // Make sure that this callsite is part of a function.
+  BasicBlock *BB = CS.getParent();
+  if (!BB)
----------------
spatel wrote:
> majnemer wrote:
> > Any chance this logic could live in `llvm::isKnownNonNullAt`
> Hmm...yes, that would probably be better. Today's comments in the earlier linked llvm-dev thread suggest that this approach (adding attributes that enable existing analysis rather than doing more analysis) is not ideal, so we might want to abandon this entirely for simplify/combines that do the work directly.
I drafted a patch to split some of the code over to isKnownNonNullAt(), but I don't think it belongs there. In this case, we're not asking if the value is known non-null at some point; we're asking if the value is known non-null throughout because of the context instruction. So it feels like a better fit for the plain isKnownNonNull() except it could be expensive to determine this nonnull-ness from there?

I'll post an updated patch with the changes suggested by @efriedma and @mkuper that hopefully makes it clearer.


https://reviews.llvm.org/D27855