[PATCH] D30572: Remove equal BBs from a function

Jessica Paquette via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 22 11:42:38 PDT 2017


paquette requested changes to this revision.
paquette added a comment.

I think the comments in this pass could use some improvement. It would be nice to be able to get the gist of what's going on without having to read through all of the code.

Does this pass only remove identical MachineBasicBlocks that end in returns/tail calls? If so, like Matthias said, we might just have a TailMerging bug. If it's more general, then what does this pass do aside from fix the bug in the test?

Also, since this is a new pass, there should be a new test written for it as well as the modification to the test you already included.



================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:10-11
+//
+// This file implements removing of equal basic blocks if there are such BBs
+// inside a function. Only one of such BBs leaves in th function
+//
----------------
I don't really understand this comment. It makes it sound like what you're trying to do is remove any two equal basic blocks in the program. Maybe expand on it a bit and give some motivation? It would make the rest of the code easier to understand.

It also might be good to rename the pass for similar reasons; if I saw this as a command line option I might be a little confused.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:71
+
+static MachineBasicBlock *hasLayoutPredecessor(MachineBasicBlock *MBB) {
+  for (auto *Pred : MBB->predecessors()) {
----------------
MBB ought to never be null, so you can pass it as a reference.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:72
+static MachineBasicBlock *hasLayoutPredecessor(MachineBasicBlock *MBB) {
+  for (auto *Pred : MBB->predecessors()) {
+    if (Pred->isLayoutSuccessor(MBB))
----------------
Generally try to avoid use of auto when the type isn't some huge template soup. In more complicated functions, it makes it a bit clearer what's going on.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:78
+}
+
+static bool Mark4Remove(SmallMapVector<unsigned, unsigned, 16> &BBsForRemove,
----------------
This function should probably have a function explaining what it's doing and the responsibilities of each parameter.

Also, a bunch of these could probably be references rather than pointers. That'd make it clear when things can't be null.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:100-101
+#ifndef NDEBUG
+  // The Replacement (MBB4Replace) could be removed "recursivly"
+  // that's why we need a loop below
+  int second = 0;
----------------
Might seem a tad pedantic, but comments should be full sentences. :) Also, watch out for spelling/grammar errors.

http://llvm.org/docs/CodingStandards.html#commenting


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:106
+    DEBUG(dbgs() << (second++ ? "\t" : "") << "REQBB: MBB4Replace #"
+                 << MBB4Replace->getNumber()
+                 << " was replaced with BB#" << V << "\n");
----------------
There are enough instances of `MBB4Replace->getNumber()` around here that it might be worth it to just give it its own variable. Say, `MBB4ReplaceNumber` or something?


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:130
+      continue; // The MBB4Replace is already successor of Pred
+    // TODO: This should be removed or it should be assertion
+    if (Pred->isSuccessor(MBB4Remove)) {
----------------
The code above or below? The stuff below alters stuff in `JTI`, while this skips `Pred` if the condition holds.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:147
+  }
+  //  assert(MBB4Remove->pred_empty() && "We can't have predecessors here");
+  if (BBsForRemove.insert({MBB4Remove->getNumber(), MBB4Replace->getNumber()})
----------------
You don't have to comment out asserts. In fact, it's a pretty good idea to use them a lot:

http://llvm.org/docs/CodingStandards.html#assert-liberally


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:165
+
+  MLI = &getAnalysis<MachineLoopInfo>();
+
----------------
This doesn't seem to be used anywhere.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:167-170
+  // Renumber all of the machine basic blocks in the function, guaranteeing that
+  // the numbers agree with the position of the block in the function.
+  // To minmize defferences in tests we'll keep current numbering
+  //F.RenumberBlocks();
----------------
This can probably all be removed?


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:177
+  }
+  // A list of BBs which will be alived after the transformation
+  SmallSet<unsigned, 16> SurvivedBBs;
----------------
Maybe "Basic blocks that will not be removed from F by the pass."


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:179-180
+  SmallSet<unsigned, 16> SurvivedBBs;
+  // A list of BBs which will be erased from the function after the
+  // transformation
+  SmallMapVector<unsigned, unsigned, 16> BBsForRemove;
----------------
Similarly, "Basic blocks that will no longer be in F at the end of the pass."


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:183
+
+  std::stable_sort(MBBs.begin(), MBBs.end(), [&](MachineBasicBlock *X,
+                                                 MachineBasicBlock *Y) {
----------------
I'd like it if `X` and `Y` were named more descriptively if only to remind me what they are later in the code. Even `MBB1` and `MBB2` or `BB1` and `BB2` would be helpful, since at least then I would know I'm talking about two `MachineBasicBlocks`.

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:202
+    // (optimize for size)
+    bool equal = true;
+    for (auto *S : X->successors()) {
----------------
Variable names should start with capital letters. I would also give this a better name. Maybe `EqualMBBs`?

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:242
+      // We're here: it means no one of X and Y was previously marked in any way
+      // TODO: bool X86InstrInfo::isTailCall(const MachineInstr &Inst);
+      auto XTerm = X->getFirstInstrTerminator();
----------------
Is this only an X86 pass? I would keep target-specific stuff out of the pass.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:245
+      // TODO: we should support CatchReturnInst
+      if (!(XTerm == X->instr_end() || /*isa<CatchReturnInst*>(XTerm) ||*/
+            (!XTerm->isReturn() && !XTerm->isUnconditionalBranch()))) {
----------------
If you aren't using the commented out stuff yet, I'd remove it.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:265-267
+        if (!XLayoutPred && YLayoutPred) {
+          std::swap(X, Y);
+        }
----------------
If there's only one statement in your `if`, you don't need the braces.


================
Comment at: lib/CodeGen/RemoveEqualBB.cpp:293-294
+    }
+    // To minmize defferences in tests we'll keep current numbering
+    //F.RenumberBlocks();
+    return true;
----------------
This seems like it can be removed.


https://reviews.llvm.org/D30572





More information about the llvm-commits mailing list