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

[CIR][CIRGen][TBAA] Initial TBAA support #1116

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

PikachuHyA
Copy link
Collaborator

This is the first patch to support TBAA, following the discussion at #1076 (comment)

  • add skeleton for CIRGen, utilizing decorateOperationWithTBAA
  • add empty implementation in CIRGenTBAA
  • introduce CIR_TBAAAttr with empty body
  • attach CIR_TBAAAttr to LoadOp and StoreOp
  • no handling of vtable pointer
  • no LLVM lowering

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch for breaking this up, mostly LGTM with few nits

clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/ternary.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@seven-mile seven-mile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Looking forward to it~

clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenDecl.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenModule.cpp Outdated Show resolved Hide resolved
@PikachuHyA
Copy link
Collaborator Author

Summary of Changes

  • Added alias printing for CIR_TBAAAttr.
  • Moved TBAA printing to the end of the output.
  • Unified variable names to use camelCase.
  • Revised comments for clarity.
  • Removed unnecessary debug output.

Explanations

  • Why is the type of TBAA mlir::ArrayAttr?
    The introduction of the tbaa attribute as OptionalAttr<SymbolRefArrayAttr> is in the commit llvm/llvm-project@edfefd7 . The follow-up changes made in llvm/llvm-project@1dda134 further address this design choice, which changes tbaa attribute as OptionalAttr<LLVM_TBAATagArrayAttr>.

    The rationale behind this decision stems from the following comment:

    // LLVM IR currently does not support attaching more than one TBAA access tag
    // to a memory accessing instruction. While supporting this in the future may
    // be beneficial, for now we simply ignore the metadata if the MLIR operation
    // has multiple access tags.

    For additional context, please refer to ModuleTranslation.cpp#L1101-L1109.

    In this patch, we use mlir::ArrayAttr to simplify the TBAA attribute, allowing for a quicker landing of the patch. We will refine the TBAA attribute design in the next patch.

@bcardosolopes
Copy link
Member

  • In this patch, we use mlir::ArrayAttr to simplify the TBAA attribute, allowing for a quicker landing of the patch. We will refine the TBAA attribute design in the next patch.

I dislike optional because there are two levels on null checking here (the optional could be empty and the array could also be empty), which is quite confusing. Better yet that LLVM doesn't really support multiple entries. I suggested to you in previous comments to create a new attr if necessary to wrap the array exactly for this reason: the array becomes an implementation detail and it doesn't really matter if we remove the array later, since it's internal state of the attribute.

Anyways, I'm fine if this comes as incremental work though. Thanks

@bcardosolopes bcardosolopes merged commit e57a9da into llvm:main Nov 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants