Page MenuHomePhabricator

[AST] Generalize argument specific aliasing
ClosedPublic

Authored by reames on Aug 14 2018, 12:43 PM.

Details

Summary

AliasSetTracker has special case handling for memset, memcpy and memmove which pre-existed argmemonly on functions and readonly and writeonly on arguments. This patch generalizes it using the AA infrastructure to any call correctly annotated.

The motivation here is to cut down on confusion, not performance per se. For most instructions, there is a direct mapping to alias set. However, this is not guaranteed by the interface and was not in fact true for these three intrinsics *and only these three intrinsics*. I kept getting myself confused about this invariant, so I figured it would be good to clearly distinguish between a instructions and alias sets. Calls happened to be an easy target.

The nice side effect is that custom implementations of memset/memcpy/memmove - including wrappers discovered by IPO - can now be optimized the same as builts by LICM.

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Aug 14 2018, 12:43 PM

I am concerned that these changes didn't change anything in behavior of dedicated tests in test/Analysis/AliasSet. I think we should commit some of those as NFC and then show how this patch changes behavior of these tests.

lib/Analysis/AliasSetTracker.cpp
443 ↗(On Diff #160668)

Aren't we supposed to return some value here?

mkazantsev added inline comments.Aug 14 2018, 8:18 PM
lib/Analysis/AliasSetTracker.cpp
436 ↗(On Diff #160668)

How about

if (!isRefSet(ArgMask))
  Access = isModSet(ArgMask) ? AliasSet::ModAccess : AliasSet::NoAccess;
438 ↗(On Diff #160668)

Looks more logical to set NoAccess as default and set ModRefAccess if both Ref and Mod are set, but it's up to you.

Thanks for the patch.

include/llvm/Analysis/MemoryLocation.h
113 ↗(On Diff #160668)

Maybe just remove this function?

lib/Analysis/AliasSetTracker.cpp
443 ↗(On Diff #160668)

the function returns void...

lib/Analysis/MemoryLocation.cpp
162 ↗(On Diff #160668)
TLI && CS.getCalledFunction() && ....
reames added inline comments.Aug 17 2018, 3:08 PM
include/llvm/Analysis/MemoryLocation.h
113 ↗(On Diff #160668)

I would prefer not to. There are a bunch of callers and I'd like to eventually pipe TLI though here as well. I consider this a temporary step.

reames updated this revision to Diff 162245.Aug 23 2018, 12:10 PM

rebase and incorporate suggestions.

There are a couple of key changes here:

  1. I'm intentionally leaving in the redundant memset, and memtransfer pieces. I'll remove this in a following NFC, but the code changes were contributing to rebase problems and weren't strictly needed. Same with removing the invariant_start from addUnknown.
  2. I removed the AA change in favour of doing this directly in the caller. This turned out to be needed for invariant_start handling (added in the rebase), and the docs for the function I was modifying contradicted my change anyway. This is much better isolated.
  3. All of the tests have landed, so only the updates are shown.

In general, I'm really unhappy with how the write-for-control-flow-only handling (guards, assumes, invariant_starts) is leaking out into more places. For the moment, I'd like to land this as is, but I plan to take a closer look at the framing there and see if we can better abstract this.

reames updated this revision to Diff 162246.Aug 23 2018, 12:14 PM

with the right patch this time

For the record, using ">>" instead of ">" when replacing an existing patch diff leads to a (surprisingly) valid patch, just not with any sensible contents.

reames added inline comments.Aug 24 2018, 9:34 AM
lib/Analysis/AliasSetTracker.cpp
459 ↗(On Diff #162246)

I noticed there's a much better way to get a ModRefInfo from a FuncitonModRefBehavior. I'll replace this stretch of code with a call to createModRefInfo(MRB);

reames updated this revision to Diff 163208.Aug 29 2018, 3:38 PM

rebase for minor issue I commented on, also ping?

anna accepted this revision.Sep 4 2018, 8:15 AM

LGTM.

lib/Analysis/AliasSetTracker.cpp
457 ↗(On Diff #163208)

which two family of intrinsics? Are these the memset and memtransfer calls? They are handled above. Maybe move the comment to above line 451.

This revision is now accepted and ready to land.Sep 4 2018, 8:15 AM
reames added inline comments.Sep 7 2018, 10:33 AM
lib/Analysis/AliasSetTracker.cpp
457 ↗(On Diff #163208)

The volatile comment is stale and will be removed before landing. AST no longer has a notion of volatile at all.

This revision was automatically updated to reflect the committed changes.