[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

First, I really think we should have a control condition collection/handling interface. So whatever loop fusion does, if this is tested we should merge it.

I went through all but the test code now. I have some comments that we need to discuss and then I'll go over the tests so we can wrap this up.



================
Comment at: llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h:81
+                           const DominatorTree &DT,
+                           const PostDominatorTree &PDT);
+
----------------
Nit: The comment is out of date.


================
Comment at: llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h:84
+  /// Return true if Conditions is empty.
+  bool empty() const { return Conditions.empty(); }
+
----------------
`empty` is fine but it's the "container view" on this.
We should have an alternative or additional function to provide the "control condition view. I mean, if this means that `ToBB` has the same control conditions as `FromBB` we should spell it out that way.


================
Comment at: llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h:96
+  /// equals to \p True. Return true if inserted successfully.
+  bool addControlCondition(const Value &V, const bool True);
+
----------------
The `True` name and the comment is confusing me. Is `True` different from `IsTrueCondition` elsewhere? If not, keep the same name. Maybe  even accept a `ControlCondition` here to avoid duplicating the internals, e.g., what makes up a `ControlCondition`.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:46
+        return true;
+    }
+
----------------
Nit: Add a TODO above to look at invariant loads and readnone calls.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:54
+          Cmp1->getOperand(1) == Cmp2->getOperand(0))
+        return true;
+
----------------
You can recurse here for the operands.

Also add a TODO that mentions other extensions (e.g., boolean binary operations) and asks someone to look into reusing GVN/CSE logic here.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:130
+      Conditions.addControlCondition(*BI->getCondition(), false);
+    }
+
----------------
The dom tree queries (`DT.dominates(BI->getSuccessor(1), CurBlock)`) confuse me. The block that contains BI is the immediate dominator of CurBlock, right?
I might be mistaken here but doesn't this mean these queries are only true if the successor and cur block are the same?

If I'm right, code like the one below and a query %from -> %to should fail with the assertion above.:
```
from:
  br label %idom
idom:
  br i1 %c0, label %lvl0a, label %lvl0b
lvl0a:
  br i1 %c1, label %lvl1a, label %lvl1b
lvl0b:
  br i1 %c1, label %lvl1b, label %lvl1c
lvl1a:
  br label %to
lvl1b:
  ret void
lvl1c:
  br label %to
to:
  ret void
```

For now you can give up if the idom is not post dominated by the current block and not the predecessor block of the current one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578