[CIR] Make AST attributes accessible via interfaces. #250

xlauko · 2023-09-02T07:06:40Z

Introduces CIR/Interfaces/ASTAttrInterfaces which model API of clang AST nodes, but allows to plugin custom attribute, making CIR dialect AST independent.
Extends hierarchy of DeclAttrs to model Decl attributes more faithfully.
Notably all CIRASTAttrs are now created uniformly using makeAstDeclAttr which builds corresponding Attribute based on clang::Decl.

clang/lib/CIR/Dialect/IR/CIRAttrs.cpp

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.td

bcardosolopes

This is great work @xlauko, I'm excited to see progress connecting our projects!

Direction looks good, thanks a lot of putting this PR up, first round of reviews are in inline comments!

clang/include/clang/CIR/Dialect/IR/CIROps.td

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.h

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.td

clang/lib/CIR/CodeGen/CIRGenBuilder.h

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.td

bcardosolopes · 2023-09-05T18:31:28Z

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.td

+        let methods = [
+            InterfaceMethod< "", "bool", "isLambda", (ins), [{}],
+                /*defaultImplementation=*/ [{
+                    return $_attr.getAstDecl()->isLambda();


Thanks for mapping at length all the stuff we currently use from the AST. As we move into encoding some extra information directly into CIR, I believe we might slowly get rid of some of these, and this will naturally serve as documentation of how much AST we end up using all around.

Is there any nodes you've added for the sake of completeness or is all of this actually used in CIR? I'd suggest we keep this list only reflecting what we currently use, so we don't unnecessarily bloat out the interface with things we don't yet have a use case for.

This was everything I needed to compile and pass tests. I only added additional intermediate Interfaces from AST hierarchy, which are empty in such case. Also to replicate API of ASTs I put methods to corresponding Interfaces, even though you do not use the interface directly, but inherit the method from base interface, e.g. NamedDecl provide getDeclName which you use only through VarDeclInterface.

clang/lib/CIR/Dialect/IR/CIRAttrs.cpp

bcardosolopes

Thanks for applying a bunch of changes, few more comments!

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

clang/lib/CIR/CodeGen/CIRGenBuilder.h

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.td

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

bcardosolopes · 2023-09-07T21:01:30Z

Regarding clang-format: there's a mode where it only apply formatting changes to the actual changes you make, otherwise fixing random things in a file generate noise for reviewers because unrelated changes. It's fine to leave them around for this patch, but if you end up creating more content, please use that mode.

xlauko · 2023-09-08T07:27:44Z

Yeah, sorry for that, I was expecting everything else was formatted. I noticed that only after push.

xlauko · 2023-09-08T13:28:04Z

@bcardosolopes Hopefully everything is fixed and rebased :) check-clang passes (at least on my machine) :D

bcardosolopes

This is great, LGTM with some minor changes from inline comments.

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.h

clang/lib/CIR/Dialect/Transforms/LifetimeCheck.cpp

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

xlauko · 2023-09-18T06:28:46Z

@bcardosolopes I rebased to current main, fixing conflicts in missing bitfield tests. Nothing else changed, so feel free to merge.

- Introduces `CIR/Interfaces/ASTAttrInterfaces` which model API of clang AST nodes, but allows to plugin custom attribute, making `CIR` dialect AST independent. - Extends hierarchy of `DeclAttr`s to model `Decl` attributes more faithfully. - Notably all `CIRASTAttr`s are now created uniformly using `makeAstDeclAttr` which builds corresponding Attribute based on `clang::Decl`.

xlauko force-pushed the ast-attr-interfaces branch from 3dd6fec to ad0d916 Compare September 2, 2023 12:01

htyu reviewed Sep 5, 2023

View reviewed changes

clang/lib/CIR/Dialect/IR/CIRAttrs.cpp Outdated Show resolved Hide resolved

clang/include/clang/CIR/Dialect/IR/CIRTypes.td Outdated Show resolved Hide resolved

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.td Outdated Show resolved Hide resolved

bcardosolopes requested changes Sep 5, 2023

View reviewed changes

bcardosolopes reviewed Sep 7, 2023

View reviewed changes

xlauko force-pushed the ast-attr-interfaces branch 5 times, most recently from 832d40f to 6c59cba Compare September 8, 2023 13:27

bcardosolopes approved these changes Sep 13, 2023

View reviewed changes

clang/include/clang/CIR/Interfaces/ASTAttrInterfaces.h Outdated Show resolved Hide resolved

clang/lib/CIR/Dialect/Transforms/LifetimeCheck.cpp Outdated Show resolved Hide resolved

clang/include/clang/CIR/Dialect/IR/CIRTypes.td Outdated Show resolved Hide resolved

xlauko force-pushed the ast-attr-interfaces branch 3 times, most recently from 58623c4 to 6a650bb Compare September 15, 2023 11:53

xlauko added 12 commits September 18, 2023 08:12

[CIR] Introduce AST attribute interfaces.

e7cae09

[CIR][Interfaces] Setup hierarchy for Decl interfaces.

6699584

[CIR] Include AST interfaces into dialect.

85d4d62

[CIR] Fix default implementation of AST interfaces.

e841f46

[CIR][IR] Introduce AST type attributes mimicing its interfaces.

f33288a

[CIR][IR] Fix names and clean up interface.

f1ba4e8

[CIR] Fix recursive cmake dependencies of attr interfaces.

fc057c9

[CIR] Add AST interface methods to access record information.

c948095

[CIR][IR] Let StructType use AST arbitrary attribute.

0a046c5

[CIR][CIRGen] Use ASTCXXRecordDeclAttr in record builder.

7a9c0e7

[CIR] Remove generated attribute printers and parsers.

7b14005

[CIR][Lifetime] Use ASTDeclInterface instead of direct AST access.

563248e

xlauko force-pushed the ast-attr-interfaces branch from 6a650bb to ac1f8b7 Compare September 18, 2023 06:18

[CIR] Update expected AST attributes names in tests.

6305a99

xlauko added 12 commits September 18, 2023 08:20

[CIR] Replace VarDeclAttr with the corresponging interface.

c84a092

[CIR] Replace FunctionDeclAttr with the corresponging interfaces.

7a3b7ab

[CIR] Clean up decl interfaces.

eea1390

[CIR] Unify AST attr construction.

41ee671

[CIR] Add TLS and Tag kind to AST Interface.

ac519c1

[CIR] Apply formatting to attr interface changes.

dcecbed

[CIR] Add interface method to query promise type existence.

94c26b2

[CIR] Use ASTVarDeclInterface to constrain AST in VarOp and GlobalOp.

82c8886

[CIR] Add missing tablegen dependencies.

784333e

[CIR] Make AST Function attributes constrained.

81e9219

[CIR] Refactor AST record interface and make it more type safe.

019b556

[CIR][IR] Remove duplicit optional of struct ast parameter.

d022d25

xlauko force-pushed the ast-attr-interfaces branch from ac1f8b7 to d022d25 Compare September 18, 2023 06:20

htyu merged commit 2d8bd16 into llvm:main Sep 18, 2023

gitoleg mentioned this pull request Sep 20, 2023

[CIR][Bugfix] adds missing clangAST dependency #267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR] Make AST attributes accessible via interfaces. #250

[CIR] Make AST attributes accessible via interfaces. #250

xlauko commented Sep 2, 2023

bcardosolopes left a comment

bcardosolopes Sep 5, 2023

xlauko Sep 6, 2023

bcardosolopes left a comment

bcardosolopes commented Sep 7, 2023 •

edited

Loading

xlauko commented Sep 8, 2023

xlauko commented Sep 8, 2023 •

edited

Loading

bcardosolopes left a comment

xlauko commented Sep 18, 2023

[CIR] Make AST attributes accessible via interfaces. #250

[CIR] Make AST attributes accessible via interfaces. #250

Conversation

xlauko commented Sep 2, 2023

bcardosolopes left a comment

Choose a reason for hiding this comment

bcardosolopes Sep 5, 2023

Choose a reason for hiding this comment

xlauko Sep 6, 2023

Choose a reason for hiding this comment

bcardosolopes left a comment

Choose a reason for hiding this comment

bcardosolopes commented Sep 7, 2023 • edited Loading

xlauko commented Sep 8, 2023

xlauko commented Sep 8, 2023 • edited Loading

bcardosolopes left a comment

Choose a reason for hiding this comment

xlauko commented Sep 18, 2023

bcardosolopes commented Sep 7, 2023 •

edited

Loading

xlauko commented Sep 8, 2023 •

edited

Loading