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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Just to get the obvious issues out of the way...



================
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()));
----------------
These need to be fetched from the pass manager instead.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1701
+    DT.recalculate(*(I->getFunction()));
+    LoopInfoBase<BasicBlock, Loop> LI;
+    LI.releaseMemory();
----------------
Should be just `LoopInfo`.


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


================
Comment at: llvm/test/Transforms/Reassociate/reassociate-not-from-the-outside-of-the-loop.ll:1
 ; RUN: opt -passes=reassociate -S < %s | FileCheck %s
 
----------------
Should use update_test_checks.py.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151616