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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

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

> 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.


It seems that `FunctionAttrs.cpp::addArgumentAttrs()` is doing the same level of thing:

In https://reviews.llvm.org/D27855#627211, @chandlerc wrote:

> In https://reviews.llvm.org/D27855#627203, @spatel wrote:
>
> > Patch updated:
> >  Move the logic and test cases from InstCombine to FuncAttrs to avoid sensitivity to function visit ordering, but this exposes a different ordering problem: -functionattrs is currently run after -inline, so this doesn't appear to help the motivating case at all.
>
>
> I don't think this is an issue of pass ordering. IMO, the issue is that we're losing information when inlining rather than preserving it. For example, your patch will catch cases where the called function *isn't* inlined. The patch is doing the correct thing, its just that you need two strategies here, not just one:
>
> 1. Handle case where secondary call is inlined.
> 2. Handle case where secondary call is not inlined.
>
>   Your patch handles #2 but not #1.
>
>   This is also somewhat abusing functionattrs IMO. You don't need this to be a function attribute at all.


Uh? How do you handle 2) without an attribute? It seems fundamentally that we want to do bottom-up inter-procedural propagation here, which is what attributes are for AFAIK.

Are you saying that relying on the inter-procedural analysis for the purpose of intra-procedural analysis is not "the right thing to do"? I'd agree, but that does not invalidate the use case of inter-procedural though, right?

> I think it would be better to teach valuetracking to look at whether a value is passed to an argument attributed 'nonnull' and/or teach the inliner to introduce an assume when inlining a function with a nonnull argument attribute to avoid losing that information.

That seems complementary to the "infer attribute approach", or I missed something?

> If you want to keep the functionattrs change to try and propagate this across long distances of procedure calls, I'm all down with that. We should be able to do this kind of bottom-up inference. But it isn't really the right way to solve your proximate use case IMO, you should use that to solve more complex cases further down the road.

I think what you're saying here is matching my comments above, but I was not totally sure to parse it correctly.


https://reviews.llvm.org/D27855