[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

This revision was automatically updated to reflect the committed changes. Closed by commit rG8bf4c1f4fb25: Reapply [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff. (authored by asbirlea). Changed prior to commit: https://reviews.llvm.org/D77341?vs=280326=280590#toc

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea added a comment. Great, thank you for confirming :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77341/new/ https://reviews.llvm.org/D77341 ___ cfe-commits mailing list

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

nikic added a comment. Numbers for the newest version: https://llvm-compile-time-tracker.com/compare.php?from=183342c0a9850e60dd7a004b651c83dfb3a7d25e=eabcf534fe8760d14c95b40f07c61450c819d643=instructions This is now a geomean 0.2% regressions, and I don't see any large outliers for individual

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea added a comment. Yes. The difference in perf in instructions and cycles count is due to reversing the update order. I'm still not seeing meaningful changes in wall-time even with the update order reversed, but reliable wall-time testing is hard. I do have changes that - drop the

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added a comment. In D77341#2171305 , @asbirlea wrote: > The increase in number of instructions and cycles was caused by reversing the > order in which the updates are applied by GraphDiff. > I'll look into re-adding some of the original

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea updated this revision to Diff 280326. asbirlea added a comment. The increase in number of instructions and cycles was caused by reversing the order in which the updates are applied by GraphDiff. I'll look into re-adding some of the original cleanups in a follow-up patch. Repository:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

nikic added a comment. In D77341#2144974 , @asbirlea wrote: > Thank you for the testing. Could you help with with instructions on how to > run the tracker myself? > My local testing showed a negligible regression for mafft and a negligible >

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea added a comment. Thank you for the testing. Could you help with with instructions on how to run the tracker myself? My local testing showed a negligible regression for mafft and a negligible improvement on other benchmarks, so it looked like noise on average. Repository: rG LLVM

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

nikic added a comment. Numbers for the new patch: https://llvm-compile-time-tracker.com/compare.php?from=c0308fd154f9a945608bd42630dc81dce5edfb40=e6e3534e77961cfa65d36828de5c75f36a25d009=instructions The regression is definitely smaller now, but still fairly large. E.g. > 2% on mafft.

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea updated this revision to Diff 276891. asbirlea added a comment. Nit: re-add `const`s Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77341/new/ https://reviews.llvm.org/D77341 Files: llvm/include/llvm/IR/Dominators.h

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea updated this revision to Diff 276890. asbirlea added a comment. This revision is now accepted and ready to land. Updated to include the part of the patch that's moving the Updates to a CFGDiff object. Splitting off from the clean-up work merging the two branches when BUI is null. This

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea reopened this revision. asbirlea added a comment. This revision is now accepted and ready to land. Herald added a project: LLVM. I'm working to update this revision to address the compile-time regression. Re-opening so I can rebase and add misc fixes and will mark as changes planned

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea updated this revision to Diff 261404. asbirlea added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77341/new/ https://reviews.llvm.org/D77341 Files: clang/include/clang/Analysis/Analyses/Dominators.h

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

nikic added a comment. This change causes a 2.5% compile-time regression across the board, see http://llvm-compile-time-tracker.com/compare.php?from=37bcf2df01cfa47e4509a5d225a23e2ca95005e6=a90374988e4eb8c50d91e11f4e61cdbd5debb235=instructions. Can you please try to find a cheaper way to

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" mehdi_amini wrote: > dblaikie wrote: > > mehdi_amini wrote: > > >

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" dblaikie wrote: > mehdi_amini wrote: > > This looks like a layering

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" mehdi_amini wrote: > This looks like a layering violation to me here:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" This looks like a layering violation to me here: I wouldn't expect

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

mehdi_amini added a comment. In D77341#1973827 , @bondhugula wrote: > @asbirlea This has broken the MLIR build. Could you please build with > `-DLLVM_ENABLE_PROJECTS=mlir`? If you use arcanist, you get pre-merge testing on Phabricator :) Repository:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

bondhugula added a comment. @asbirlea This has broken the MLIR build. Could you please build with `-DLLVM_ENABLE_PROJECTS=mlir`? FAILED: tools/mlir/lib/Analysis/CMakeFiles/MLIRAnalysis.dir/Dominance.cpp.o /usr/lib64/ccache/clang++ -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=1

Re: [PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

I'm hoping rG5da1671bf823 fixes them. Please let me know if not and I will revert. Alina On Thu, Apr 9, 2020 at 7:03 PM Stella Stamenova via Phabricator < revi...@reviews.llvm.org> wrote: > stella.stamenova added a comment. > > It looks like this broke the windows lldb buildbot: > >

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

stella.stamenova added a comment. It looks like this broke the windows lldb buildbot: lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/15542 It already had a test failure, so you probably didn’t get the email. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

This revision was automatically updated to reflect the committed changes. Closed by commit rGa90374988e4e: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff. (authored by asbirlea). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea marked 2 inline comments as done. asbirlea added a comment. Thank you for all the comments. Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:325 +auto = BUI ? BUI->PreViewCFG : EmptyGD; +return !empty(children({, N})); }

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

kuhar accepted this revision. kuhar added a comment. This revision is now accepted and ready to land. Looks good to me overall. I don't want to block it over the cosmetic issues like allocating the empty GD object. Comment at:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea marked 4 inline comments as done. asbirlea added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:325 +auto = BUI ? BUI->PreViewCFG : EmptyGD; +return !empty(children({, N})); } kuhar wrote: > This pattern

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea updated this revision to Diff 255852. asbirlea marked an inline comment as done. asbirlea added a comment. Name anonymous namespace. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77341/new/ https://reviews.llvm.org/D77341 Files:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/CFGDiff.h:198 +namespace { +template kuhar wrote: > kuhar wrote: > > What benefit does an anonymous namespace in a header have over a named one, > > e.g., `detail`/`impl`? Doesn't it make it

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

kuhar added inline comments. Comment at: llvm/include/llvm/IR/CFGDiff.h:198 +namespace { +template kuhar wrote: > What benefit does an anonymous namespace in a header have over a named one, > e.g., `detail`/`impl`? Doesn't it make it more difficult to

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

kuhar added a comment. In D77341#1960854 , @asbirlea wrote: > Address comments. Thanks for the changes and explanations. It think with a few more tweaks this will be a good refactoring step towards phasing out BUI. Comment at:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea updated this revision to Diff 254943. asbirlea marked 16 inline comments as done. asbirlea added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77341/new/ https://reviews.llvm.org/D77341 Files:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:185 + std::make_pair(GDTmp, BB); + for (auto : children(GDNodePair)) { +const NodePtr Succ = Pair.second; kuhar wrote: > Won't children

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

kuhar added inline comments. Comment at: llvm/include/llvm/IR/CFGDiff.h:199 +namespace { +template struct reverse_if_helper; +template <> struct reverse_if_helper { You can use two helper functions taking `integral_constant` tags -- should be less verbose

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

kuhar added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:186 + for (auto : children(GDNodePair)) { +const NodePtr Succ = Pair.second; const auto SIT = NodeToInfo.find(Succ); kuhar wrote: > Could this

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added a comment. Couple of small nits, but I'll leave most of the review to someoen else here - I /think/ it's beyond my context/experience (but if necessary, poke me and I can look more/ask more questions/etc) Comment at:

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea added a comment. I sent this out to get some general feedback, but I'd like to be able to simplify the fact that this is replacing a `for` over `ChildrenGetter` with a `for` over `children(pair)`, plus 5 lines to set up the type and pair; one simplification is around getting the type

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

asbirlea created this revision. asbirlea added reviewers: kuhar, dblaikie, NutshellySima. Herald added subscribers: cfe-commits, hiraditya. Herald added a project: clang. asbirlea added a comment. asbirlea added a parent revision: D77167: [GraphDiff] Extend GraphDiff to track a list of updates..