[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur 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,
----------------
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?


================
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:
> 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`.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:36
+/// Represent a control condition. A control condition is a condition of a
+/// terminaotr to decide which successors to execute. The pointer field
+/// represents the address of the condition of the terminator. The integer field
----------------
[typo] terminaotr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578