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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemoryLocation.h:272
+                          LocationSize Size =
+                              LocationSize::unknownNonNegative(),
                           const AAMDNodes &AATags = AAMDNodes())
----------------
asbirlea wrote:
> Default to the most generic? `unknownMaybeNegative`
I have dropped this default entirely in https://github.com/llvm/llvm-project/commit/393b9e9db31a3f83bc8b813ee24b56bc8ed93a49. We should not assume one way or another.


================
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()) {
----------------
asbirlea wrote:
> Could you clarify the TODO?
This TODO is for D91482. I've just dropped the TODO entirely as I committed the hasValue() usage separately.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1637
+    // TODO: Relax GEP checks above and use unknownMaybeNegative().
+    PNSize = LocationSize::unknownNonNegative();
 
----------------
asbirlea wrote:
> 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.
That's right, I've expanded the TODO a bit. I think we should leave this at unknownNonNegative() for now, as changing it to unknownMaybeNegative() might technically regress results, so it seems like something to evaluate separately.


================
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));
----------------
asbirlea wrote:
> For checks in loops using AST, I'd use `LocationSize::unknownMaybeNegative()`.
>From cursory inspection, I didn't see any obvious requirement that the loads be increasing, so I agree that this should use unknownMaybeNegative().


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

https://reviews.llvm.org/D91649