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

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 19 17:00:46 PST 2016


spatel added a comment.

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


Thanks for the guidance! I agree that this a special case, but I thought I'd try this one first for the learning experience. On that count at least, progress. :)

On the associated llvm-dev thread, we've discussed related solutions involving llvm.assume and ValueTracking, so yes, I'll see what I can do with those too. And the interplay of analysis and (abuse of) attributes was also nicely explained.

Hopefully, I'll pick this back up in a couple of weeks, but if anyone else is interested (dyn_cast!), feel free to push ahead.


https://reviews.llvm.org/D27855





More information about the llvm-commits mailing list