Page MenuHomePhabricator

Fix issues required to remove memset/memcpy/memmove special casing from AliasSetTracker
AbandonedPublic

Authored by reames on Sep 10 2018, 3:53 PM.

Details

Summary

The heart of this change is an NFC which removes dedicated handling in AST for handling mem* intrinsics in favour of the recently added generic argmemonly handling.

However, doing this exposed two deeper issues which are definitely not NFC:

  1. We'd never sunk the handling for the atomic.elementwise variants down into MemoryLocation (which is the codepath used by the generic argmemonly work)
  2. We weren't treating memmove as being writeonly in it's first argument. This is really the change which concerns me. (Note: memmove allows aliasing src and dest, but that doesn't allow the we can read *through* the dest argument which is how the parameter attributes are specified.)

Together, these two make the change mildly risky and I figured an extra set or two of eyes was definitely warranted.

Diff Detail

Event Timeline

reames created this revision.Sep 10 2018, 3:53 PM
reames added inline comments.Sep 10 2018, 4:16 PM
lib/Analysis/MemoryLocation.cpp
126

Realized I'd missed the atomic memset case. I added new tests (in test/Analysis/AliasSet/memset.ll) which would have caught this and will rebase at some point.

reames planned changes to this revision.Jan 25 2019, 10:11 AM

Not something I'm actively working on. Will split apart when I get back to this for easier review.

reames abandoned this revision.Sat, Oct 19, 2:40 PM

No longer actively working on this, may return to it in the future.

Herald added a project: Restricted Project. · View Herald TranscriptSat, Oct 19, 2:40 PM