[PATCH] D86156: [BFI] Preserve BFI information through loop passes via VH callbacks inside LoopStandardAnalysisResults

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.
Herald added a subscriber: danielkiss.

New compile-time numbers: https://llvm-compile-time-tracker.com/compare.php?from=d7c119d89c5f6d0789cfd0a139c80e23912c0bb0&to=e0a1a6cac1b982023f8ceba8285d1ee7bc96bd32&stat=instructions The regression is now reduced to 0.2%. I assume that's the overhead of those CallbackVHs.

I have no familiarity with BFI, so possibly stupid question: There is already some similar handling as part of BFIImpl here: https://github.com/llvm/llvm-project/blob/0f14b2e6cbb54c84ed3b00b0db521f5ce2d1e3f2/llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h#L1043-L1058 What is the difference to that / why are both needed?



================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfo.h:43
+  class BFICallbackVH final : public CallbackVH {
+    std::weak_ptr<ImplType> BFI;
+    const BasicBlock *BB;
----------------
Is the shared_ptr/weak_ptr usage here necessary? This type of map deletion CallbackVH is a common pattern, but this is the first time I see it with weak_ptr.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfo.h:44
+    std::weak_ptr<ImplType> BFI;
+    const BasicBlock *BB;
+
----------------
It should not be necessary to explicitly store `BB` here, it is already part of the value handle (you can access it via `*this` for example).


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfo.h:48
+    BFICallbackVH(std::shared_ptr<ImplType> BFI, const BasicBlock *BB);
+    virtual ~BFICallbackVH() = default;
+
----------------
I don't believe this virtual dtor is needed (class is final). For that matter, the method below also don't need to marked virtual.


================
Comment at: llvm/lib/Analysis/BlockFrequencyInfo.cpp:212
+  for (const auto *BB : RegisteredBlocks)
+    BlockWatchers.emplace_back(BFICallbackVH(BFI, BB));
+
----------------
May want to clear BlockWatchers in `BlockFrequencyInfo::releaseMemory()`?


================
Comment at: llvm/lib/Transforms/Scalar/LoopDistribute.cpp:1062
+    LoopStandardAnalysisResults AR = {AA,  AC,  DT,      LI,     SE,
+                                      TLI, TTI, nullptr, nullptr};
     return LAM.getResult<LoopAccessAnalysis>(L, AR);
----------------
Huh, surprised that clang-format allows this.


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

https://reviews.llvm.org/D86156