This is an archive of the discontinued LLVM Phabricator instance.

returned - BasicAA should look through functions with returned arguments
ClosedPublic

Authored by hfinkel on Apr 30 2015, 8:31 AM.

Details

Summary

Motivated by the work on the llvm.noalias intrinsic, teach BasicAA to look through returned-argument functions when answering queries. This is essential so that we don't loose all other AA information when supplementing with llvm.noalias.

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 24717.Apr 30 2015, 8:31 AM
hfinkel retitled this revision from to llvm.noalias - BasicAA should look through them too.
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added reviewers: chandlerc, reames, dberlin.
hfinkel added a subscriber: Unknown Object (MLST).

Also, the ability to optionally strip llvm.noalias calls as pointer casts added here is also used in several later patches.

dberlin added inline comments.Apr 30 2015, 8:56 AM
include/llvm/IR/Value.h
404

So, is there actually a place you plan on not passing true here, or are you just trying to keep existing logic where you haven't explicitly modified it?

(Because i am having a bit of trouble seeing where you would want to pass false. At the point you are stripping, why is noalias more magical to not strip than anything else that can appear here?)

hfinkel added inline comments.Apr 30 2015, 10:12 AM
include/llvm/IR/Value.h
404

In some sense both, but I also wanted to keep the default not to strip them.

At least in theory, looking through casts and stripping llvm.noalias calls could lead to rewriting the pointer in terms of some underlying object (thus removing the aliasing information) -- SROA might do this for example -- and so I thought it would be easier to vet specific uses that we know won't do this.

Generically, I thought that "I added restrict and my code slowed down" will be easier to correct than "I added restrict and my code did not speed up as it should have".

We could certainly change the default (and thus remove the need for several of the patches in this series ;) ). I don't have a strong opinion.

dberlin added inline comments.May 12 2015, 1:04 PM
include/llvm/IR/Value.h
404

After thinking about this a lot, i'm strongly of the opinion we should just change the default. I think anything else is likely to create a mess :)

In fact, i'd go further, and say this shouldn't be a parameter at all. If someone wants a function that stops at noalias, they should write a new strip function. I don't think noalias is special enough, and can't think of a real use case, for wanting to stop there.

reames resigned from this revision.Oct 8 2015, 10:25 AM
reames removed a reviewer: reames.

Resigning as a reviewer to get a very stale review off my list of blocking tasks in phabricator. Please reopen when desired.

hfinkel updated this revision to Diff 63333.Jul 8 2016, 3:35 PM

Rebased. As Danny requested, the Boolean parameters to stripPointerCasts have been removed; they now always look through the noalias intrinsics. This means that we need to prevent InstCombine and friends from removing the noalias intrinsics from the pointer-value use-def chains some other way. As a result, this particular patch is now much simpler.

majnemer accepted this revision.Jul 10 2016, 12:38 AM
majnemer added a reviewer: majnemer.
majnemer added a subscriber: majnemer.

LGTM

lib/IR/Value.cpp
418

auto *I

477

Ditto.

This revision is now accepted and ready to land.Jul 10 2016, 12:38 AM
hfinkel updated this revision to Diff 63438.Jul 10 2016, 2:53 PM
hfinkel retitled this revision from llvm.noalias - BasicAA should look through them too to returned - BasicAA should look through functions with returned arguments.
hfinkel updated this object.
hfinkel edited edge metadata.

Changed from being specific to llvm.noalias to applying to all returned-argument functions.

Nice, thanks!

This revision was automatically updated to reflect the committed changes.