https://github.com/bcardosolopes requested changes to this pull request.
Direction is right, I think it's reasonable to not depend on AST in the lowering pipeline, but this shouldn't preclude AST usage and require proper user experience in case AST isn't available. Agreeing with Erich on the split, and both Linux bots are red, so this needs another round regardless. Not repeating Andy's consistency points, they're solid. Three things from me. **Where the line is.** `LowerModule` carrying `TargetInfo` and `LangOpts` off the triple is fine, that's a handful of scalars, not AST duplication. Per-decl attributes are the ones I'll push on. `#cir.static_local_info` exists, and your reply to Andy proposes more of the same shape. For module-level facts, go ahead. What I don't want is a parallel per-decl AST growing in the dialect one fact at a time, where each pass that used to ask the AST a question gets its own attribute. If a pass needs something per-decl, the `$ast` backreference stays the primary and the materialized attr is the serialization fallback, not the other way round. **Keep the backreferences.** Nothing here removes `$ast` or `#cir.var.decl` and I checked that on purpose, the in-process path is unaffected. Say that explicitly in the commit message, because "run without a live ASTContext" reads like "AST backreferences are going away" and they aren't. **Diagnostics, not asserts.** The `static_local_info` change is the one I want to be the rule for the whole series. The moment `.cir` is an input format, every invariant the pipeline used to get from the AST is reachable from a file someone hand-wrote or an older compiler emitted. An assert is not a diagnostic and disappears in release. Each fact you stop reading from the AST needs a real error when it isn't there. Layering comments inline, including one on the `include_directories` hack that I don't think should land as-is. https://github.com/llvm/llvm-project/pull/219048 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
