[PATCH] D120177: [BOLT] CMOVConversion pass

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks! Nicely written pass! There are a couple of lint issues and I have some comments, but this is mostly ready to go in my opinion.



================
Comment at: bolt/include/bolt/Passes/CMOVConversion.h:37
+
+/// Pass for duplicating blocks that would require a jump.
+class CMOVConversion : public BinaryFunctionPass {
----------------
I guess this comment was copied from another pass?


================
Comment at: bolt/include/bolt/Passes/CMOVConversion.h:40
+  /// Record how many possible cases there are.
+  uint64_t PossibleConversions = 0;
+
----------------
Can we also record instruction frequency and come up with a dynamic metric such as "out of 10000 executed instructions that could be converted to cmov, we converted 5000 (50%)"?


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:66
+    return false;
+  return true;
+}
----------------
Isn't it necessary to also check that LHS has a common predecessor with RHS?  Edit: I saw how the main function uses this and this assumption is implied. Perhaps add a comment "caller guarantees that LHS and RHS share the same predecessor".


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:101
+    if (BC.MIB->isUnconditionalBranch(*II)) {
+      BB.eraseInstruction(II);
+      return;
----------------
I know this is returning so it doesn't matter, but it makes me uneasy to think that someone might copy this code and remove the return. I would expect something like:

  auto II = BB.begin();
  while (II != BB.end()) {
    if (BC.MIB->isPseudo(*II)) {
      ++II;
      continue;
    }
    if (BC.MIB->isUnconditionalBranch(*II)) {
      II = BB.eraseInstruction(II);
      continue;
    }
    ...
    ++II;
  }

But that might be an overkill, so we can add a comment for the incautious reader "invalidates the iterator, but that's OK since we return now"


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:111
+
+bool matchCFGSubgraph(BinaryBasicBlock &BB, BinaryBasicBlock *&ConditionalSucc,
+                      BinaryBasicBlock *&UnconditionalSucc,
----------------
Can we move this functions right after isIfThenSubgraph, closer to the helper that it uses, or vice-versa (move isIfThenSubgraph above)?


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:155
+  // successor.
+  auto RPO = ReversePostOrderTraversal<BinaryFunction *>(&Function);
+  bool Modified = false;
----------------
Is this necessary? Can't we just traverse in BF->BB vector order?


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:166
+
+    assert(BB->isValid() && "Traversal internal error");
+
----------------



================
Comment at: bolt/lib/Target/X86/X86MCPlusBuilder.cpp:2143
+    // CMOV can only be a load (filter out store-moves below)
+    if (isLoad(Inst)) {
+      // If stack memory operands are not allowed, bail early
----------------
If we are only converting loads and you are already filtering stores below (I guess this is done in the switch-case of line 2163), can't we just drop this if and run the checks in its body regardless of it being load/store?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120177