[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Florian Hahn via Phabricator via llvm-commits]

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)
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578