[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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)
----------------
fhahn wrote:
> fhahn wrote:
> > 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.
> Ah I see this was added after another review comment. I don't have a very strong opinion on this, but I don't really see a benefit of exposing this, as long as there are no other users. Exposing it once there's a need is trivial and keeping it private makes it slightly easier to change/adapt.
> 
> If you want to expose this interface, I think that's best done in a separate review, to keep the reviews focused (e.g. the title/description of this patch do not mention the new interface and the motivation at all).
> Ah I see this was added after another review comment.

Correct, I explicitly asked to do that.


> I don't have a very strong opinion on this, but I don't really see a benefit of exposing this, as long as there are no other users. Exposing it once there's a need is trivial and keeping it private makes it slightly easier to change/adapt.

We can obviously move it back to the .cpp file now and back to the header once the outside user comes in. To be honest, I would rather not put it in CodeMoveUtils to begin with but in sth like ControlConditons.h, that would make the discussion if it should be open obsolete as well. Anyway, since it can be moved later and you seem to dislike this solution, I will not argue to expose it any more.


> If you want to expose this interface, I think that's best done in a separate review, to keep the reviews focused (e.g. the title/description of this patch do not mention the new interface and the motivation at all).

Changing the title/description of the commit would seem easy enough though


================
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:
> 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 ;)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578