[PATCH] D91649: [AA] Split up LocationSize::unknown()

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

- `LocationSize::unknownMaybeNegative()` seems somewhat ambiguous in isolation; unless I see the other name or have the context from this patch. I'm either keep `LocationSize::unknown` or make it even more verbose `LocationSize::unknownPositiveOrNegative()`. I think the most restrictive case ( `LocationSize::unknownMaybeNegative()` or renamed version) should be the default `~uint64_t(0)`, but don't feel strongly about it.
- clang-format



================
Comment at: llvm/include/llvm/Analysis/MemoryLocation.h:272
+                          LocationSize Size =
+                              LocationSize::unknownNonNegative(),
                           const AAMDNodes &AATags = AAMDNodes())
----------------
Default to the most generic? `unknownMaybeNegative`


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1386
       // stripped a gep with negative index ('gep <ptr>, -1, ...).
-      if (V1Size != LocationSize::unknown() &&
-          V2Size != LocationSize::unknown()) {
+      // TODO: The V2Size.hasValue() check is unnecessary.
+      if (V1Size.hasValue() && V2Size.hasValue()) {
----------------
Could you clarify the TODO?


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1637
+    // TODO: Relax GEP checks above and use unknownMaybeNegative().
+    PNSize = LocationSize::unknownNonNegative();
 
----------------
This is not `unknownMaybeNegative()` because of the above check `!cast<ConstantInt>(PVGEP->idx_begin())->isNegative()` ?

I would set this to `unknownMaybeNegative()` and leave the TODO.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:521
     Value *Ptr = const_cast<Value*>(Loc.Ptr);
-    AST.add(Ptr, LocationSize::unknown(), Loc.AATags);
+    AST.add(Ptr, LocationSize::unknownNonNegative(), Loc.AATags);
     Accesses.insert(MemAccessInfo(Ptr, false));
----------------
For checks in loops using AST, I'd use `LocationSize::unknownMaybeNegative()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91649/new/

https://reviews.llvm.org/D91649