[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 20 00:37:35 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)
----------------
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
More information about the llvm-commits
mailing list