Page MenuHomePhabricator

llvm.noalias - GetUnderlyingObjects to optionally collect noalias calls
AcceptedPublic

Authored by hfinkel on Apr 30 2015, 9:17 AM.

Details

Summary

This is part of the series started by D9375, and lets GetUnderlyingObjects, optionally, collect the llvm.noalias calls that it has looked through. This will be used by the AA implementation for llvm.noalias, to avoid double-walking (which could be expensive) and copy-and-paste.

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 24737.Apr 30 2015, 9:17 AM
hfinkel retitled this revision from to llvm.noalias - GetUnderlyingObjects to optionally collect noalias calls.
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added reviewers: chandlerc, reames.
hfinkel added a subscriber: Unknown Object (MLST).
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 63331.Jul 8 2016, 3:29 PM

Rebased.

I've thought about how to generalize this (it seems kind of silly to have GetUnderlyingObject(s) have a special parameter for this), but because I don't think that imposing the overhead of a virtual function call for each visited instruction is reasonable, I think the way to generalize it is to template it on some kind of customization functor. I've done that here, in the name of keeping this change small, and because I don't yet have a second use case. I'm happy to do the generalization either sequenced before this change or afterward (assuming, of course, we don't decide on a completely different way to satisfy this use case).

majnemer added inline comments.
include/llvm/Analysis/ValueTracking.h
227–235

Perhaps the NoAlias parameter deserves a mention?

lib/Analysis/ValueTracking.cpp
3070

auto *I

3070

Should we assign V the value of I->getOperand(0) ?

3070–3071

The following might be a little shorter:

if (match(V, m_Intrinsic<Intrinsic::noalias>())
3071

Braces are unneeded.

hfinkel added inline comments.Jul 9 2016, 5:00 PM
include/llvm/Analysis/ValueTracking.h
227–235

Yes. I'll add test to the comment.

lib/Analysis/ValueTracking.cpp
3070

Yes, this happens in D9383.

3070

auto *I

Sure.

3071

Both true, but I think it makes more sense this way once D9383 is also applied.

hfinkel updated this revision to Diff 63406.Jul 9 2016, 5:02 PM

Updated per review comments.

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

LGTM with nits

include/llvm/Analysis/ValueTracking.h
228–235

I find it more likely that a SmallVector will get passed in instead of a custom MaxLookup. Perhaps they should be reordered to one another?

This revision is now accepted and ready to land.Jul 10 2016, 12:31 AM

Looks like patch was not committed.

hfinkel updated this revision to Diff 73348.Oct 3 2016, 2:53 PM
hfinkel edited edge metadata.

Rebased

majnemer added inline comments.Oct 3 2016, 2:58 PM
lib/Analysis/ValueTracking.cpp
3070–3074

I think you can sink this into the block for the auto CS = CallSite(V) condition. You can compare CS.getIntrinsicID() with Intrinsic::noalias.

hfinkel added inline comments.Oct 3 2016, 3:14 PM
lib/Analysis/ValueTracking.cpp
3070–3074

Yes, indeed!

hfinkel updated this revision to Diff 73351.Oct 3 2016, 3:15 PM

Addressed review feedback.

troyj added a subscriber: troyj.Aug 22 2018, 9:30 AM