[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 20 00:28:09 PST 2019


fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h:31
+/// terminaotr to decide which successors to execute.
+struct ControlCondition {
+  ControlCondition(const Value *V, bool IsTrueCondition)
----------------
Is there a strong need to expose all those implementation details in the header here? 

It seems this is just an implementation detail used for isControlFlowEquivalent & co that can be entirely contained in CodeMoverUtils.cpp. Unless there's a convincing use case that requires those details to be exposed, I think we should keep them in the .cpp.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:101
+            !IsDifferent(Other.getFalseConditions(), FalseConditions,
+                         TrueConditions));
+  }
----------------
Whitney wrote:
> jdoerfert wrote:
> > What if you would only track positive conditions (negate the others) and you put them into a set. The is equivalent check will then be imple equality check on the sets. You can then also check "implication" through subset relation, and missing condition, through set difference.
> I changed to use unordered_set. I created a ControlCondition struct instead as the type for the unordered_set. Do you think that's sufficient?
This should use DenseMap, unless there is a good justification to not use it. Also ControlCondition could just be a tagged pointer (http://llvm.org/doxygen/classllvm_1_1PointerIntPair.html)


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:38
+
+// FIXME: Reuse GVN/CSE logic to check for equivalence between Values.
+bool llvm::ControlConditionsComparator::isEquivalent(const Value &V1,
----------------
I guess we could just use SCEV here to check for equivalence. I don't think re-using GVN logic will be feasible, unless you want to value number the whole function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578





More information about the llvm-commits mailing list