[PATCH] D157068: [Attributor] New attribute to identify what byte ranges are alive for an allocation

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3551-3553
+    if (auto *AI = dyn_cast<AllocaInst>(&I)) {
+      getOrCreateAAFor<AAAllocationInfo>(IRPosition::value(*AI));
+    }
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:874
   const_bin_iterator end() const { return OffsetBins.end(); }
+  int64_t size() const { return OffsetBins.size(); }
 
----------------
Call it numBins or sth, to make it clear. Size could be a lot of things in this context.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12208
+
+    auto *I = this->getIRPosition().getCtxI();
+    AllocaInst &AI = cast<AllocaInst>(*I);
----------------
no this->


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12210
+    AllocaInst &AI = cast<AllocaInst>(*I);
+    const IRPosition &IRP = IRPosition::inst(AI);
+    auto *PI =
----------------
This is just getIRPosition().


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12218
+
+    const AAPointerInfo &OtherAA = *PI;
+
----------------
Why would we want to have the same value twice, use the pointer.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12220-12222
+    if (!OtherAA.getState().isValidState()) {
+      return indicatePessimisticFixpoint();
+    }
----------------
Style, also elsewhere.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12224
+
+    const auto &OtherAAImpl = static_cast<const AAPointerInfoImpl &>(*PI);
+    const auto &State = OtherAAImpl.getState();
----------------
We should not cast things to the Impl. Expose the functions you need in the AAPointerInfo interface instead. 


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12240
+    int64_t OffsetEnd = It->getFirst().Offset + It->getFirst().Size;
+    const DataLayout &DL = AI.getModule()->getDataLayout();
+    const auto &AllocationSize = AI.getAllocationSize(DL);
----------------
I think A.getDataLayout() is a thing too.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12265-12267
+    if (!isValidState()) {
+      return indicatePessimisticFixpoint();
+    }
----------------
Make this an assertion, manifest will not be called on invalid states.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157068