[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! There a few remaining small comments from my side. It would probably best to wait a bit with committing, in case someone else has additional thoughts.

In terms of algorithm, it might be slightly faster to interleave condition discovery and checks, i.e. collect the conditions for the first BB, then while collecting conditions for the second BB bail out once we encounter a condition that's not also in the list of the first BB. But that's a minor thing and could be done as a follow up.



================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:52
+class ControlConditions {
+  using SelfTy = SmallVector<ControlCondition, 6>;
+
----------------
nit: I am not sure if SelfTy here is as descriptive as it could be. I would expect SelfTy to match the type of the enclosing class or similar. Maybe something like ConditionVectorTy would be slightly better (although I am not too concerned about the name).


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:208
+      if (Cmp1->getPredicate() ==
+              CmpInst::getSwappedPredicate(Cmp2->getInversePredicate()) &&
+          Cmp1->getOperand(0) == Cmp2->getOperand(1) &&
----------------
nit: should we cover the swapped case in isEquivalent?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:320
+  DT.updateDFSNumbers();
+  const bool MoveForward = OI.dfsBefore(&I, &InsertPoint);
   if (MoveForward) {
----------------
nit: I think it would be clearer to use OI.dominates, like t line 333. I think with OI.dominates, the DFS numbers should be updated on demand, automatically.


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:13
 #include "llvm/Analysis/LoopInfo.h"
+#include "llvm/Analysis/OrderedInstructions.h"
 #include "llvm/Analysis/PostDominators.h"
----------------
Is this required here?


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:61
+  std::unique_ptr<Module> M =
+      parseIR(C, "define void @foo(i32* dereferenceable(4) %i, i1 zeroext "
+                 "%cond1, i1 zeroext %cond2) {\n"
----------------
I think you should be able to avoid creating the string line by line with \n using R"", as in https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Transforms/Utils/LocalTest.cpp#L183


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:64
+                 "entry:\n"
+                 "  %frombool1 = zext i1 %cond1 to i8\n"
+                 "  %frombool2 = zext i1 %cond2 to i8\n"
----------------
is there a reason for the frombool/tobool clutter here and in the other tests? Could we not just pass `i1 %cond1` and use that directly? Also I think the `dereferenceable(4) ` metadata can be dropped.


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:90
+        FI++;
+        BasicBlock *FirstIfBody = &*(FI++);
+        assert(FirstIfBody->getName() == "if.first" &&
----------------
Given how often this is used to get a pointer to a basic block by name I think it would be worth adding a getBasicBlockByName helper, similar to https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Analysis/ScalarEvolutionTest.cpp#L210


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578