[PATCH] D120177: [BOLT] CMOVConversion pass

[Amir Ayupov via Phabricator via llvm-commits]

Amir marked an inline comment as done.
Amir added inline comments.


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:66
+    return false;
+  return true;
+}
----------------
rafauler wrote:
> 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".
Added both


================
Comment at: bolt/lib/Passes/CMOVConversion.cpp:155
+  // successor.
+  auto RPO = ReversePostOrderTraversal<BinaryFunction *>(&Function);
+  bool Modified = false;
----------------
rafauler wrote:
> Is this necessary? Can't we just traverse in BF->BB vector order?
Yes, post-order traversal is necessary to guarantee that we can collapse the hammock into a straight-line code (merge Predecessor, RHS and LHS into a single BB). Actually, I've made a mistake here as we need to traverse bottom-up (post-order, not RPO). Added a back-to-back hammocks test for that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120177