[PATCH] D44748: Track whether the size of a MemoryLocation is precise
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 19 15:58:49 PDT 2018
reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.
Mostly minor comments. I considered giving a conditional LGTM, but decided it warranted one last round. I think this is very close to landing.
================
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:
> > 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.
================
Comment at: include/llvm/Analysis/MemoryLocation.h:112
+ // `Other`.
+ LocationSize combineWith(LocationSize Other) const {
+ if (Other == *this)
----------------
Can we pick a different name? Maybe: union?
This is returning a result which is conservatively correct for uses of either. combineWith doesn't make that obvious.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1718
bool NullIsValidLocation = NullPointerIsDefined(&F);
- if ((V1Size != MemoryLocation::UnknownSize &&
- isObjectSmallerThan(O2, V1Size.getValue(), DL, TLI,
- NullIsValidLocation)) ||
- (V2Size != MemoryLocation::UnknownSize &&
- isObjectSmallerThan(O1, V2Size.getValue(), DL, TLI,
- NullIsValidLocation)))
+ if ((V1Size.isPrecise() && isObjectSmallerThan(O2, V1Size.getValue(), DL, TLI,
+ NullIsValidLocation)) ||
----------------
This is definitely a warranted functional change, but please add a test case which reflects the change.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1779
if (O1 == O2)
- if (V1Size != MemoryLocation::UnknownSize &&
- V2Size != MemoryLocation::UnknownSize &&
+ if (V1Size.isPrecise() && V2Size.isPrecise() &&
(isObjectSize(O1, V1Size.getValue(), DL, TLI, NullIsValidLocation) ||
----------------
Same testing comment here.
================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:1140
+ } else {
+ // This query's Size is less than the cached one. Conservatively restart
+ // the query using the greater size.
----------------
This looks to have very different behaviour than previously. Unless I'm misreading nesting, you’re recomputing any time the sizes are equal which is odd.
https://reviews.llvm.org/D44748
More information about the llvm-commits
mailing list