[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle abandoned this revision. nhaehnle added a comment. Herald added a subscriber: teijeong. Superseded by D92924 , D92925 , D92926 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. Herald added a subscriber: dexonsmith. I'm going to follow up with another RFC about this on llvm-dev. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/ https://reviews.llvm.org/D83088

[PATCH] D83088: Introduce CfgTraits abstraction

rengolin added a comment. I have read all of the comments in this review and have looked at all the other pending reviews because of this and I still see strong disagreement on the design and implementation. Unfortunately, I can't contribute with the technical discussion because there's a lot

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. I replied on llvm-dev. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/ https://reviews.llvm.org/D83088 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D83088: Introduce CfgTraits abstraction

mehdi_amini added a comment. In D83088#2350287 , @nhaehnle wrote: > Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have > a general rule that patches can be reverted if they're obviously broken (e.g. > build bot problems) or

[PATCH] D83088: Introduce CfgTraits abstraction

arsenm added a comment. In D83088#2348641 , @mehdi_amini wrote: > In D83088#2347111 , @arsenm wrote: > >> In D83088#2346322 , @mehdi_amini >> wrote: >> >>> In

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have a general rule that patches can be reverted if they're obviously broken (e.g. build bot problems) or clearly violate some other standard. This is a good rule, but it doesn't apply here. If

[PATCH] D83088: Introduce CfgTraits abstraction

mehdi_amini added a comment. In D83088#2347111 , @arsenm wrote: > In D83088#2346322 , @mehdi_amini > wrote: > >> In D83088#2345540 , @nhaehnle wrote: >> >>> David, I don't

[PATCH] D83088: Introduce CfgTraits abstraction

arsenm added a comment. In D83088#2346322 , @mehdi_amini wrote: > In D83088#2345540 , @nhaehnle wrote: > >> David, I don't think this is appropriate here. Let's take the discussion to >> llvm-dev. > > Seems like

[PATCH] D83088: Introduce CfgTraits abstraction

mehdi_amini added a comment. In D83088#2345540 , @nhaehnle wrote: > David, I don't think this is appropriate here. Let's take the discussion to > llvm-dev. Seems like David asked to revert in the meantime? Repository: rG LLVM Github Monorepo

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. David, I don't think this is appropriate here. Let's take the discussion to llvm-dev. Comment at: mlir/include/mlir/IR/Dominance.h:49 +template <> +struct llvm::CfgTraitsFor { + using CfgTraits = mlir::CfgTraits; antiagainst wrote:

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie reopened this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sorry about the delays in review - but please revert this patch until we can hash out a few more details. I really don't think this is the best direction forward for a core abstraction &

[PATCH] D83088: Introduce CfgTraits abstraction

antiagainst added inline comments. Comment at: mlir/include/mlir/IR/Dominance.h:49 +template <> +struct llvm::CfgTraitsFor { + using CfgTraits = mlir::CfgTraits; rriddle wrote: > This seems to have broken the GCC5 build: >

[PATCH] D83088: Introduce CfgTraits abstraction

rriddle added inline comments. Comment at: mlir/include/mlir/IR/Dominance.h:49 +template <> +struct llvm::CfgTraitsFor { + using CfgTraits = mlir::CfgTraits; This seems to have broken the GCC5 build:

[PATCH] D83088: Introduce CfgTraits abstraction

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc0cdd22c72fa: Introduce CfgTraits abstraction (authored by nhaehnle). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. Herald added a subscriber: rdzhabarov. ping^2 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/ https://reviews.llvm.org/D83088 ___ cfe-commits mailing list

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. Herald added a subscriber: tatianashp. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/ https://reviews.llvm.org/D83088 ___ cfe-commits mailing list

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added inline comments. Comment at: llvm/include/llvm/Support/CfgTraits.h:51 + + operator bool() const { return ptr != nullptr; } + dblaikie wrote: > `operator bool` should be `explicit` Done. Comment at:

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle updated this revision to Diff 287441. nhaehnle added a comment. - cleanup operators on CfgOpaqueType - address other review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/ https://reviews.llvm.org/D83088 Files:

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. The most immediate problem is divergence analysis, which is extremely complex and difficult to get right. If I had tried to fight the accidental complexity that comes with attempting to write such an algorithm as C++ templates in addition to the

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. (side note: this code review is a bit hard to follow with all the linting messages about naming - might be a bit more readable if it conformed to the naming conventions?) In D83088#2227151 , @nhaehnle wrote: > In

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. > Based on your description and the DomTree patches, if I understand correctly, > the primary motivation is to facilitate writing CFG-representation-agnostic > algorithms/analyses (e.g., dominators, divergence, convergence analyses), > such that you can later lift the

[PATCH] D83088: Introduce CfgTraits abstraction

kuhar added a comment. Hi Nicoali, In D83088#2227151 , @nhaehnle wrote: > ... > Take a look here for example: > https://github.com/nhaehnle/llvm-project/blob/715450fa7f968ceefaf9c3b04b47066866c97206/llvm/lib/Analysis/GenericConvergenceUtils.cpp#L499 >

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. In D83088#2225415 , @dblaikie wrote: >>> But I guess coming back to the original/broader design: What problems is >>> this intended to solve? The inability to write non-template algorithms over >>> graphs? What cost does that

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2224297 , @nhaehnle wrote: >> Not sure that's the best place to be designing this fairly integral and >> complicated piece of infrastructure from, but hoping we can find some good >> places/solutions/etc. > > I sent

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. > Not sure that's the best place to be designing this fairly integral and > complicated piece of infrastructure from, but hoping we can find some good > places/solutions/etc. I sent an email to llvm-dev several weeks ago, but things seem to have moved here. Either

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2218559 , @nhaehnle wrote: > In D83088#2213886 , @dblaikie wrote: > >> In D83088#2213864 , @nhaehnle wrote: >> >>> In D83088#2213802

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. In D83088#2213886 , @dblaikie wrote: > In D83088#2213864 , @nhaehnle wrote: > >> In D83088#2213802 , @dblaikie wrote: >> >>> In D83088#2213797

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2213864 , @nhaehnle wrote: > In D83088#2213802 , @dblaikie wrote: > >> In D83088#2213797 , @nhaehnle wrote: >> >>> In D83088#2208611

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. In D83088#2213802 , @dblaikie wrote: > In D83088#2213797 , @nhaehnle wrote: > >> In D83088#2208611 , @dblaikie wrote: >> >>> This seems like a

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2213797 , @nhaehnle wrote: > In D83088#2208611 , @dblaikie wrote: > >> This seems like a strange hybrid between a static-polymorphism (with traits) >> and dynamic polymorphism

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added a comment. In D83088#2208611 , @dblaikie wrote: > This seems like a strange hybrid between a static-polymorphism (with traits) > and dynamic polymorphism (with base classes/virtual functions). Could this > more readily be just one or the

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. This seems like a strange hybrid between a static-polymorphism (with traits) and dynamic polymorphism (with base classes/virtual functions). Could this more readily be just one or the other? (sounds like you're leaning towards dynamic polymorphism) Repository: rG

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle updated this revision to Diff 284195. nhaehnle marked 2 inline comments as done. nhaehnle added a comment. v7: - std::forward fix in wrapping_iterator - fix typos Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/

[PATCH] D83088: Introduce CfgTraits abstraction

arsenm added inline comments. Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:50 + + // Clang's CFG contains nullpointers for unreachable succesors, e.g. when an + // if statement's condition is always false, it's 'then' branch is represented

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle updated this revision to Diff 280481. nhaehnle marked an inline comment as done. nhaehnle added a comment. v6: - implement predecessors/successors for all CfgTraits implementations - fix error in unwrapRange - rename toGeneric/fromGeneric into wrapRef/unwrapRef to have naming

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle marked 3 inline comments as done. nhaehnle added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:44 +// use on a 32-bit architecture. +assert(wrapped != (uintptr_t)-1 && wrapped != (uintptr_t)-2); + arsenm wrote: > I

[PATCH] D83088: Introduce CfgTraits abstraction

arsenm added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:44 +// use on a 32-bit architecture. +assert(wrapped != (uintptr_t)-1 && wrapped != (uintptr_t)-2); + I feel like there should be a better way to do this; we should

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle marked an inline comment as done. nhaehnle added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:136-138 + // Prefer to avoid support for bundled instructions as long as we + // don't really need it. +

[PATCH] D83088: Introduce CfgTraits abstraction

arsenm added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:136-138 + // Prefer to avoid support for bundled instructions as long as we + // don't really need it. + assert(!m_instr->isBundle()); nhaehnle

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle updated this revision to Diff 275811. nhaehnle marked 4 inline comments as done. nhaehnle added a comment. - fix MachineCfgTraits::blockdef_iterator and allow it to iterate over the instructions in a bundle - use MachineBasicBlock::printName Repository: rG LLVM Github Monorepo

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:133 + ++m_def; + if (m_def == m_instr->defs().end()) { +++m_instr; arsenm wrote: > != return early? The logic is actually subtly broken in the presence of

[PATCH] D83088: Introduce CfgTraits abstraction

arsenm added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:133 + ++m_def; + if (m_def == m_instr->defs().end()) { +++m_instr; != return early? Comment at:

[PATCH] D83088: Introduce CfgTraits abstraction

nhaehnle created this revision. nhaehnle added reviewers: arsenm, RKSimon, mehdi_amini, courbet. Herald added subscribers: cfe-commits, msifontes, jurahul, Kayjukh, vkmr, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst,