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

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

In D86156#2245103 <https://reviews.llvm.org/D86156#2245103>, @nikic wrote:

> 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?

Well this is a funny situation: looks like someone else saw the same problem we observed and pushed the fix also using VH callbacks (D75341 <https://reviews.llvm.org/D75341>). Both of us coming to the same solution here is a good sign that its a good fit.

I'm glad you pointed this out! Looking at my diff with both callbacks incorporated the node gets erase() called twice but since the second call isn't in the DenseMap erase becomes a no-op. This explains why both changes didn't catastrophically collide against each other.

Given all that, the changes here to pass along the BFI information in the loop passes and allow usage in LICM still seems meaningful enough to commit. I've removed the redundant call-back added. Also updating the description to reflect the latest updates.



================
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);
----------------
nikic wrote:
> Huh, surprised that clang-format allows this.
I also thought that was a mis-format at first but the combination of rules turns out to prefer this.


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

https://reviews.llvm.org/D86156