Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement MLIR visibility in terms of CIR visibility #1029

Open
bcardosolopes opened this issue Oct 30, 2024 · 1 comment
Open

Implement MLIR visibility in terms of CIR visibility #1029

bcardosolopes opened this issue Oct 30, 2024 · 1 comment
Labels
good first issue Good for newcomers

Comments

@bcardosolopes
Copy link
Member

MLIR one handles public, private and nested, CIR instead has all we need for C/C++ semantics + LLVM lowering.

The initial plan when we introduced CIR visibility was to later remove explicit reference to MLIR one, and implement the necessary interfaces in terms of CIR instead (also because we don't want to print/parse both on globals/functions, sounds like silly and unnecessary bloating). @roro47 did the first part, which is to introduce CIR visibility, but she's not working on CIR anymore.

The missing steps from previous plan of record is (hopefully still makes sense):

  1. Add a CIR visibility attribute, print and parse and fix tests for FuncOp and GlobalOp (ignore MLIR visibility for now).
  2. Migrate CIRGen properties that are currently querying MLIR visibility, to instead query the attribute representing CIR visibility (like some linkage stuff currently does). Perhaps here you introduce extraClassDeclarations to this new attribute or add some type of CIR interface (one example is the AST interface or GlobalValue).
  3. Remove printing and parser for MLIR visibility while providing custom implementation of the interface methods here: mlir/include/mlir/IR/SymbolInterfaces.td. We need to keep sym name and visibility but the requirements are not as many: https://mlir.llvm.org/docs/SymbolsAndSymbolTables/#defining-or-declaring-a-symbol
  4. At this point MLIR visibility is only important when using the symbol interface to query information. Anything query done on top of CIR should come through it's attributes.

cc @smeenai @seven-mile

@bcardosolopes bcardosolopes added the good first issue Good for newcomers label Oct 30, 2024
@bcardosolopes
Copy link
Member Author

Fixing this should take into account the extra whitespaces for cir.global, @seven-mile already fixed cir.func in #1028

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant