[PATCH] D151616: [Transforms][Reassociate] "Reassociate expressions" pass optimizations not always profitable

[Paul Osmialowski via Phabricator via llvm-commits]

pawosm01 marked 4 inline comments as done.
pawosm01 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1699
+    // need to know how to revert this transformation in order to succeed.
+    DominatorTree DT;
+    DT.recalculate(*(I->getFunction()));
----------------
nikic wrote:
> These need to be fetched from the pass manager instead.
OK, used pass manager instead.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1701
+    DT.recalculate(*(I->getFunction()));
+    LoopInfoBase<BasicBlock, Loop> LI;
+    LI.releaseMemory();
----------------
nikic wrote:
> Should be just `LoopInfo`.
Although this place has gone, I've considered your suggestion in the other place below.



================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1707
+    // loop.
+    for (unsigned i = 0; i != Ops.size(); ++i) {
+      // Only consider expressions we're allowed to transform.
----------------
tschuett wrote:
> for (unsigned i = 0, size_t e = Ops.size(); i != e; ++i) {
> }
Thanks for spotting that!


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1732
+        return nullptr;
+      };
+
----------------
nikic wrote:
> This looks like a very complicated and inefficient re-implementation of `LI->getLoopFor()`.
using LI->getLoopFor() instead now.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151616