asbirlea added inline comments.

================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:185
+          std::make_pair(GDTmp, BB);
+      for (auto &Pair : children<GraphDiffBBPair>(GDNodePair)) {
+        const NodePtr Succ = Pair.second;
----------------
kuhar wrote:
> Won't children infer the template parameter based on the passes value? I 
> don't get why the template argument type is a pair where the second argument 
> is a directed nodeptr, but the runtime value is always a plain nodeptr. 
> Couldn't the second pair type also be directed for `GDNodePair`?
The directed nodeptr is the equivalent of a bool inside GraphDiff translating 
to what kind of children do you want; the second pair argument bears no info of 
that kind, only the actual NodePtr for which we desire the children.


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:186
+      for (auto &Pair : children<GraphDiffBBPair>(GDNodePair)) {
+        const NodePtr Succ = Pair.second;
         const auto SIT = NodeToInfo.find(Succ);
----------------
kuhar wrote:
> kuhar wrote:
> > Could this new code be a hoisted into a helper function?
> Or alternatively, could the old `ChildrenGetter` be implemented with these 5 
> magic lines?
I think it looks much cleaner after the latest update, let me know what you 
think.


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:1543
+  // FIXME: Updated to use the PreViewCFG and behave the same as until now.
+  // This behavior is however incorrect; this actually needs the PostViewCFG.
+  GraphDiff<typename DomTreeT::NodePtr, DomTreeT::IsPostDominator> PreViewCFG(
----------------
kuhar wrote:
> Does this care about the direction (pre- or post-) at all, or does it need 
> some CFG view? Why is pre-view incorrect?
The purpose of this method was to update the DT assuming an additional list of 
updates (the argument given). In practice, the BUI argument is ignored in the 
`CalculateFromScratch` call below. Hence we need the additional changes to 
support this kind of updates inside `CalculateFromScratch` and the other 
methods.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to