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

Mon Dec 19 16:11:42 PST 2016

chandlerc added a comment.

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.

I think the transformation this patch is trying to do is exactly what the "assume" framework tries to do -- you have one point in your code that makes an assumption valid (nonnull) and you want to use it later in that same code. The fact that in some cases you can promote this to an attribute on the argument is kind of incidental.

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.

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.

https://reviews.llvm.org/D27855