This is an archive of the discontinued LLVM Phabricator instance.

[FunctionAttr] Infer nonnull attributes on returns
ClosedPublic

Authored by reames on May 11 2015, 4:48 PM.

Details

Summary

Teach FunctionAttr to infer the nonnull attribute on return values of functions which never return a potentially null value. This is done both via a conservative local analysis for the function itself and a optimistic per-SCC analysis. If no function in the SCC returns anything which could be null (other than values from other functions in the SCC), we can conclude no function returned a null pointer. Even if some function within the SCC returns a null pointer, we may be able to locally conclude that some don't.

This is inspired by discussion on http://reviews.llvm.org/D9132, but is mostly orthogonal to that patch except that they both relate to nonnul attributes.

Diff Detail

Event Timeline

reames updated this revision to Diff 25527.May 11 2015, 4:48 PM
reames retitled this revision from to [FunctionAttr] Infer nonnull attributes on returns.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added reviewers: nlewycky, pete, hfinkel.
reames added a subscriber: Unknown Object (MLST).
pete edited edge metadata.May 11 2015, 5:05 PM

Hi Philip

I made a few comments, but mostly just about using foreach and some comments. Nothing major.

Otherwise, LGTM.

Cheers,
Pete

lib/Transforms/IPO/FunctionAttrs.cpp
73

I think you need \brief here

77

And here, with \\\ and without listing the function name.

849

All function types are pointer types. I think you need F->getReturnType()->isPointerTy() as you have below.

854

I think this can just be a foreach loop.

858

I think this would be better as a worklist you pop values off until its empty.

866

I think this can be a dyn_cast so that you can fold it with the cast<> below.

887

I don't know if there's a foreach on the phi values. If there is then good to use it, otherwise I guess someone else can add it later.

nlewycky added inline comments.May 11 2015, 8:22 PM
lib/Transforms/IPO/FunctionAttrs.cpp
858

Be careful not to break the case of resolving a self recursive phi. I think the normal way to handle this is with a set of "visited" elements, plus a worklist "queue" like Pete suggests.

920

"Speculative assume" -> "Speculatively assume" or possibly just "Speculate"

test/Transforms/FunctionAttrs/nonnull.ll
61

Extra newline.

Responding to comments. Any comment not explicitly addressed with be fixed before submission.

lib/Transforms/IPO/FunctionAttrs.cpp
849

Thanks. Good catch.

854

Yep, I copied this from code above. If you don't mind, I'll leave this as is, then fix both in one patch.

858

As Nick pointed out, the use of the SetVector is important here. Given this is how other algorithms in the same file are structured, I'd prefer not to change this.

887

I copied this from code above. If you don't mind, I'll leave this as is, then fix both in one patch.

pete added a comment.May 12 2015, 2:34 PM

SGTM. I have no problems with a separate patch. Thanks!

This revision was automatically updated to reflect the committed changes.