[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Looking at the examples, it looks like GVN + GVNHoist would at least some of the equivalences trivial. GVNHoist is currently disabled by default, but I think it would be a more general way to materialize equivalences similar to the ones in the tests, rather than teaching various places to do their own equivalence checks.



================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:54
+  /// A unordered_set of control conditions.
+  SelfTy Conditions;
+
----------------
Comment Needs update


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:73
+  /// Return a constant reference of Conditions.
+  const SelfTy &getControlConditions() const { return Conditions; }
+
----------------
Not sure if that is really needed?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:76
+  /// Return the basic block range associated with this ControlConditions.
+  std::pair<const BasicBlock *, const BasicBlock *> getRange() const {
+    return std::make_pair(&FromBB, &ToBB);
----------------
Not used?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:86
+  /// equivalent control condition in \p Other.Conditions.
+  bool isEquivalent(const ControlConditions &Other) const;
+
----------------
I think this potentially can be quite expensive. IMO it would be good to have a note about that here.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:171
+
+  return llvm::all_of(Conditions, [&Other](const ControlCondition &C) {
+    return any_of(Other.getControlConditions(),
----------------
Similar to my suggestion for isEquivalent(const Value &V1, const Value &V2), I think it would be good to have a limit for the number of conditions to collect to avoid compile-time explosion on IR triggered the worst case here.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:192
+// Values.
+bool ControlConditions::isEquivalent(const Value &V1, const Value &V2) {
+  if (&V1 == &V2)
----------------
I think it would be good to have a limit here on the number of recursions, like we have in many places where we walk back the def-use-chains, to avoid compile-time explosions on IR triggering the worst case.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:202
+  // The result of a LoadInst or CallBase can be different at different
+  // point in time.
+  // TODO: Add handle of invariant loads and readnone calls.
----------------
nit: different points in time?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:225
+
+  for (const Value *Op1 : I1->operands())
+    for (const Value *Op2 : I2->operands())
----------------
This seems to be comparing every operand from I1 with every operand from I2. Wouldn’t it be enough to compare the first op of both, the second operands and so on? Would also be good to have a test case.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:331
 
+/// Return true if \p I0 comes before \p I1.
+static bool isMoveForward(const Instruction &I0, const Instruction &I1,
----------------
This comment should say what it means that I0 comes before I1. Also, the name isMoveForward seems unconnected to the description.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:337
+         "Expecting I0 and I1 to be control flow equivalent");
+
+  if (DT.dominates(&I0, &I1) && PDT.dominates(&I1, &I0))
----------------
Using DT to compare instruction ordering a is quite inefficient. OrderedInstructions is better for multiple queries. I'm not sure if it works with PDT though


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578