[PATCH] D44748: Track whether the size of a MemoryLocation is precise

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/MemoryLocation.h:35
+// it contains, N, is 'precise'. Precise, in this context, means that we know
+// that the MemoryLocation we're referencing has to be at least N bytes large.
+//
----------------
george.burgess.iv wrote:
> reames wrote:
> > george.burgess.iv wrote:
> > > reames wrote:
> > > > I think you mean: must be exactly N bytes.  If not, I'm confused.
> > > Yeah, I could've worded this better. I was trying to say "block of bytes that the `MemoryLocation` references" (e.g. a `store i32 1, i32* %a` implies that `%a` must be *at least* 4 bytes large). Will update with the rebase.
> > Looks like this update got lost?  
> > 
> > JFYI, I consider clarity on this point essential. 
> Good catch; sorry for dropping this. Please let me know if you'd like me to expand further.
Reading through this again, I think we have a semantic problem here.  The previous semantics of MemoryLocation was that we had a memory region which was guaranteed to be at least as large as the memory actually dereferenced.  Your documented new semantics now indicate that the accessed region is *larger* than the MemLoc when that MemLoc is precise.  This now means that to know whether a MemLoc is inclusive of the access we *must* check the precise bit.

I am not okay with that change.

I would be okay with either of the following:
1) Precise means "accesses exactly N bytes".  (i.e. still bounded from above, but now also bounded from below with the same value)
2) We replace "precise" with a MinSize field.  A memory location would then guarantee that the actual access was bounded from below by MinSize and above by Size.

I recommend (1) for the moment since I think that's what you've actually implemented.  (2) would be more generic, but would be basically starting over.


https://reviews.llvm.org/D44748