Skip to content

[CIR][CodeGen][NFC] Refactor performAddrSpaceCast to consume CIR address space attributes #747

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

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

seven-mile
Copy link
Collaborator

Originally TargetCodeGenInfo::performAddrSpaceCast consumes two unused parameters typed clang::LangAS. This PR changes its type to mlir::cir::AddressSpaceAttr to better fit the need of CIRGen.

In D32248: CodeGen: Cast alloca to expected address space, the author explained why these AS parameters are not used:

This is just the default implementation. The idea is that targets that need to do something more complex on a particular conversion — e.g. to make sure that null pointers are translated correctly when they have different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing custom behaviors like above for those targets.

@seven-mile
Copy link
Collaborator Author

Motive

In order to fix alloca AS in #682 in a way that works seamlessly with CIR's unified address space, we intend to change clang::LangAS getASTAllocaAddressSpace() to mlir::cir::AddressSpaceAttr getCIRAllocaAddressSpace().

Apart from that, we have several addrspace casts to fix. For example (in OG CodeGen):

- LangAS AAS = getASTAllocaAddressSpace();
+ mlir::cir::AddressSpaceAttr AAS = getCIRAllocaAddressSpace();
  
- LangAS EAS = E->getType()->getPointeeType().getAddressSpace();
+ mlir::cir::AddressSpaceAttr EAS = builder.getAddrSpaceAttr(
+      E->getType()->getPointeeType().getAddressSpace());
  if (AAS != EAS) {
    // ...
    return RValue::get(getTargetHooks().performAddrSpaceCast(*this, AI, AAS,
                                                              EAS, Ty));
  }

In this code snippets, AAS has type LangAS and therefore fits the need of performAddrSpaceCast. But then we want to change its type to mlir::cir::AddressSpaceAttr. It's easy to get CIR AS from LangAS, but not vice versa. So I think this refactor provides cleaner API and do no harm to future development.

Copy link
Contributor

@jopperm jopperm left a comment

Choose a reason for hiding this comment

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

Seems like the logical next step to me.

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 the more incremental changes!

@bcardosolopes bcardosolopes merged commit 3e00be7 into llvm:main Jul 18, 2024
7 checks passed
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
…dress space attributes (llvm#747)

Originally `TargetCodeGenInfo::performAddrSpaceCast` consumes two
*unused* parameters typed `clang::LangAS`. This PR changes its type to
`mlir::cir::AddressSpaceAttr` to better fit the need of CIRGen.

In [D32248: CodeGen: Cast alloca to expected address
space](https://reviews.llvm.org/D32248), the author explained why these
AS parameters are not used:

> This is just the default implementation. The idea is that targets that
need to do something more complex on a particular conversion — e.g. to
make sure that null pointers are translated correctly when they have
different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing
custom behaviors like above for those targets.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
…dress space attributes (llvm#747)

Originally `TargetCodeGenInfo::performAddrSpaceCast` consumes two
*unused* parameters typed `clang::LangAS`. This PR changes its type to
`mlir::cir::AddressSpaceAttr` to better fit the need of CIRGen.

In [D32248: CodeGen: Cast alloca to expected address
space](https://reviews.llvm.org/D32248), the author explained why these
AS parameters are not used:

> This is just the default implementation. The idea is that targets that
need to do something more complex on a particular conversion — e.g. to
make sure that null pointers are translated correctly when they have
different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing
custom behaviors like above for those targets.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
…dress space attributes (llvm#747)

Originally `TargetCodeGenInfo::performAddrSpaceCast` consumes two
*unused* parameters typed `clang::LangAS`. This PR changes its type to
`mlir::cir::AddressSpaceAttr` to better fit the need of CIRGen.

In [D32248: CodeGen: Cast alloca to expected address
space](https://reviews.llvm.org/D32248), the author explained why these
AS parameters are not used:

> This is just the default implementation. The idea is that targets that
need to do something more complex on a particular conversion — e.g. to
make sure that null pointers are translated correctly when they have
different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing
custom behaviors like above for those targets.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
…dress space attributes (llvm#747)

Originally `TargetCodeGenInfo::performAddrSpaceCast` consumes two
*unused* parameters typed `clang::LangAS`. This PR changes its type to
`mlir::cir::AddressSpaceAttr` to better fit the need of CIRGen.

In [D32248: CodeGen: Cast alloca to expected address
space](https://reviews.llvm.org/D32248), the author explained why these
AS parameters are not used:

> This is just the default implementation. The idea is that targets that
need to do something more complex on a particular conversion — e.g. to
make sure that null pointers are translated correctly when they have
different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing
custom behaviors like above for those targets.
lanza pushed a commit that referenced this pull request Nov 5, 2024
…dress space attributes (#747)

Originally `TargetCodeGenInfo::performAddrSpaceCast` consumes two
*unused* parameters typed `clang::LangAS`. This PR changes its type to
`mlir::cir::AddressSpaceAttr` to better fit the need of CIRGen.

In [D32248: CodeGen: Cast alloca to expected address
space](https://reviews.llvm.org/D32248), the author explained why these
AS parameters are not used:

> This is just the default implementation. The idea is that targets that
need to do something more complex on a particular conversion — e.g. to
make sure that null pointers are translated correctly when they have
different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing
custom behaviors like above for those targets.
lanza pushed a commit that referenced this pull request Mar 18, 2025
…dress space attributes (#747)

Originally `TargetCodeGenInfo::performAddrSpaceCast` consumes two
*unused* parameters typed `clang::LangAS`. This PR changes its type to
`mlir::cir::AddressSpaceAttr` to better fit the need of CIRGen.

In [D32248: CodeGen: Cast alloca to expected address
space](https://reviews.llvm.org/D32248), the author explained why these
AS parameters are not used:

> This is just the default implementation. The idea is that targets that
need to do something more complex on a particular conversion — e.g. to
make sure that null pointers are translated correctly when they have
different bit-patterns — can easily do so.

Further more, I'm confident that the CIR AS is also capable of providing
custom behaviors like above for those targets.
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