[PATCH] D30572: Remove equal BBs from a function

[Davide Italiano via Phabricator via llvm-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

Is it possible to address this at IR level? Anyway, comments inline.



================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:1
+//===-- FuncletLayout.cpp - Contiguously lay out funclets -----------------===//
+//
----------------
FuncletLayout ?


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:10-12
+// This file implements basic block placement transformations which result in
+// funclets being contiguous.
+//
----------------
This comment doesn't seem quite right.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:47-48
+  }
+  /// \brief A handle to the loop info.
+  MachineLoopInfo *MLI;
+
----------------
You seem to pass MLI around but you don't use for anything real (the only line that actually uses it is commented out).


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:53
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<MachineDominatorTree>();
+    AU.addRequired<MachineLoopInfo>();
----------------
Why do you need the dominator here?


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:191-196
+      //      DEBUG(dbgs() << "REQBB: In '" << F.getName()
+      //                   << "' we have candidates for -O1: BB#" <<
+      //                   X->getNumber()
+      //                   << " and BB#" << Y->getNumber() << "\n");
+      //      DEBUG(X->dump());
+      //      DEBUG(Y->dump());
----------------
Commented out code.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:252-256
+        //        DEBUG(dbgs() << "REQBB: In BB#" << X->getNumber()
+        //                     << "we have illegal terminator preventing the
+        //                     Equal BB "
+        //                        "optimization\n";
+        //              X->dump(););
----------------
Commented out code.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:263-266
+      // F.erase(F.getBlockNumbered(Number));
+      auto RemBB = F.getBlockNumbered(Number.first);
+      // TODO: it should be done later
+      // MLI->removeBlock(RemBB);
----------------
ditto.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:667-668
   addPass(&FuncletLayoutID, false);
+  if (getOptLevel() != CodeGenOpt::None)
+    addPass(&RemoveEqualBBID, false);
 
----------------
Why are you putting at this point in the pipeline?


================
Comment at: test/CodeGen/X86/loop-search.ll:1-2
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-apple-darwin | FileCheck %s
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-apple-darwin -remove-equal-bbs | FileCheck %s
 
----------------
This needs more tests.


https://reviews.llvm.org/D30572