[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:59
+  /// Return a ControlConditions which stores all conditions required to execute
+  /// \p BB from \p Dominator. If \p MaxLookup is non-zero, it limit for the
+  /// number of conditions to collect. Return None if not all conditions are
----------------
nit: 'it limits the '?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:61
+  /// number of conditions to collect. Return None if not all conditions are
+  /// collected successfully.
+  static Optional<const ControlConditions>
----------------
'... or we hit the limit' ?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:173
+
+  return llvm::all_of(Conditions, [&Other](const ControlCondition &C) {
+    return any_of(Other.Conditions, [&C](const ControlCondition &OtherC) {
----------------
nit: no llvm:: needed? 


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:191
+
+// FIXME: Use SCEV and reuse GVN/CSE logic to check for equivalence between
+// Values.
----------------
I think it would be good to mention that we rely on other passes to ensure equivalent conditions have the same value.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:194
+bool ControlConditions::isEquivalent(const Value &V1, const Value &V2) {
+  return (&V1 == &V2);
+}
----------------
nit: no braces needed


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:316
 
-  // As I and InsertPoint are control flow equivalent, if I dominates
-  // InsertPoint, then I comes before InsertPoint.
-  const bool MoveForward = DT.dominates(&I, &InsertPoint);
+  const bool MoveForward = OrderedInstructions(&DT).dominates(&I, &InsertPoint);
   if (MoveForward) {
----------------
OrderedInstruction numbers basic block on demand and caches them. In order for that to be effective, it needs to be passed in by the caller. E.g. moveInstsBottomUp would have to create it and pass it in. It should also be used for the `dominates` checks further down the function.

But looking at the latest version of the patch, it seems to be completely orthogonal to the main changes. If that's the case, probably best to be split off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578