[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
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,
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > fhahn wrote:
> > > jdoerfert wrote:
> > > > fhahn wrote:
> > > > > 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
> > > > > I guess we could just use SCEV here to check for equivalence. 
> > > > 
> > > > "Just use SCEV" is maybe the wrong wording, at least for what I had in mind. My thinking was:
> > > > We have probably quite a few "equivalence" checker in the code base. Which one to reuse depends at the end of the day on the properties you need.
> > > > It becomes interesting as soon as you actually have condition sets that do not match 1-1 but are still equivalent. As I mentioned earlier, other relations, e.g., subset, will also be interesting. This is all "future work" where though.
> > > > 
> > > > 
> > > > > I don't think re-using GVN logic will be feasible, unless you want to value number the whole function
> > > > 
> > > > If that turns out to help normalizing complex control conditions, why not. I will hopefully have a GSoC student to revive the PolyhedralValueAnalysis, that is even more expensive ;)
> > > > 
> > > > 
> > > > "Just use SCEV" is maybe the wrong wording, at least for what I had in mind.
> > > I should have been more specific. I meant by using SCEV we would be able to handle more general conditions, benefit from normalization and probably do all that with less code and benefit from the existing caching.
> > > 
> > > 
> > > > My thinking was:
> > > > We have probably quite a few "equivalence" checker in the code base. Which one to reuse depends at the end of the day on the properties you need.
> > > > It becomes interesting as soon as you actually have condition sets that do not match 1-1 but are still equivalent. As I mentioned earlier, other relations, e.g., subset, will also be interesting. This is all "future work" where though.
> > > 
> > > That makes a lot of sense to me.
> > > 
> > > >> I don't think re-using GVN logic will be feasible, unless you want to value number the whole function
> > > > If that turns out to help normalizing complex control conditions, why not. I will hopefully have a GSoC student to revive the PolyhedralValueAnalysis, that is even more expensive ;)
> > > 
> > > Sure, those are things we can decide on driven by data. 
> > > 
> > @Whitney Comparing equivalences using `==` on SCEVPredicate only works on SCEVable types and expressions (eg. ). Would that be feasible or too limiting?
> > 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
> 
> What I was thinking was to run GVN/GVNHoist beforehand or add LoopFuse close after GVN in the pass pipeline s.t. we could assume that equivalent conditions are represented by the same `llvm::Value`.
Changed to simplify comparing the address of the two `llvm::Value`s. As mentioned, if we first run GVN/GVNHoist, then this is sufficient. We can come back to this when a use case came up to require a more complex equivalence check, or if we encounter issues with running GVN/GVNHoist before certain pass. 


================
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);
----------------
fhahn wrote:
> Not used?
Right, it is not used. This is added after a reviewer suggestion. We can add it back later when this become a public interface. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578