This is an archive of the discontinued LLVM Phabricator instance.

[AliasAnalysis] Take into account readonly attribute for the function arguments
ClosedPublic

Authored by igor-laevsky on Oct 22 2015, 12:05 PM.

Diff Detail

Event Timeline

igor-laevsky retitled this revision from to [AliasAnalysis] Take into account readonly attribute for the function arguments.
igor-laevsky updated this object.
igor-laevsky added reviewers: reames, chandlerc.
igor-laevsky added a subscriber: llvm-commits.
chandlerc added inline comments.Oct 22 2015, 2:13 PM
include/llvm/Analysis/AliasAnalysis.h
758–759 ↗(On Diff #38156)

No, the AA *base* clase should have zero logic.

See BasicAAResult for an implementation which does this (and many other things) to provide useful AA information.

igor-laevsky added inline comments.
include/llvm/Analysis/AliasAnalysis.h
758–759 ↗(On Diff #38156)

Thanks. It appears that BasicAliasAnalysis does not do this. Please see the updated diff.

hfinkel added inline comments.
lib/Analysis/BasicAliasAnalysis.cpp
619

Don't you need to check all arguments, not just the first? (please add some corresponding test cases)

igor-laevsky added inline comments.
lib/Analysis/BasicAliasAnalysis.cpp
619

Other arguments are checked inside the AAResultBase::getModRefInfo. And it seems like this is the only place which uses getArgModRefInfo.

It is indeed incorrect to assume that function does not modify argument memory based on single readonly attribute. However comment in the AAResults::Concept::getArgModRefInfo states that:

/// Note that these bits do not necessarily account for the overall behavior of
/// the function, but rather only provide additional per-argument
/// information.

So I guess it's fine to leave this check only for single argument.

I have added relevant test case.

hfinkel accepted this revision.Oct 27 2015, 5:35 PM
hfinkel added a reviewer: hfinkel.

LGTM.

In follow-up, it would be nice to handle the ReadNone attribute also.

lib/Analysis/BasicAliasAnalysis.cpp
619

Indeed, not sure what I was thinking; thanks for the test case.

This revision is now accepted and ready to land.Oct 27 2015, 5:35 PM
This revision was automatically updated to reflect the committed changes.