Skip to content

Conversation

el-ev
Copy link
Member

@el-ev el-ev commented Mar 24, 2025

This is not very useful now, as both branches of interest are NYI. I will try to implement the real features in subsequent patches.

Copy link

github-actions bot commented Mar 24, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@el-ev
Copy link
Member Author

el-ev commented Mar 24, 2025

@bcardosolopes I've done some work on ELFDependentLibraries (el-ev@a989cc2). I added an attribute for CIR, but I'm not seeing a corresponding attribute for LLVM-dialect MLIR. Is this the case where some upstream changes are required first?

@bcardosolopes
Copy link
Member

bcardosolopes commented Mar 24, 2025

Thanks for working on this, awesome!

Is this the case where some upstream changes are required first?

I see, https://llvm.org/docs/LangRef.html#dependent-libs-named-metadata is indeed not available yet.

So you'll need to add support in LLVM dialect to lower it. I'm fine if you add without LLVM support for now and file an issue to tackle lowering later (because we don't yet have the feature available). I've been making lots of similar changes to LLVM dialect upstream, you can look to them as examples if you'd like to send a PR to upstream MLIR (I can help review too).

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.

You either need to add a testcase or convey that this PR is a NFC change. The testcase would need to exercise PCK_Compiler, PCK_ExeStr and PCK_User because they are not asserting right now, so classify as a introduced feature.

@@ -7149,7 +7149,6 @@ void CodeGenModule::EmitTopLevelDecl(Decl *D) {
case Decl::ObjCCompatibleAlias:
ObjCRuntime->RegisterAlias(cast<ObjCCompatibleAliasDecl>(D));
break;

Copy link
Member

Choose a reason for hiding this comment

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

We don't change files outside CIR dir without a good reason, formatting is specially bad for rebasing, so you need to remove the changes to this file.

@el-ev el-ev changed the title [CIR][CIRGen] Handle PragmaComment in emitTopLevelDecl [CIR][CIRGen][NFC] Handle PragmaComment in emitTopLevelDecl Mar 25, 2025
@el-ev
Copy link
Member Author

el-ev commented Mar 25, 2025

Pragma comments other than PCK_Lib are dropped in the parser, so this is indeed an NFC change.

https://github.com/llvm/clangir/blob/0fd1437f1c7e901273fb19f1db27ae32c952a0b8/clang/lib/Parse/ParsePragma.cpp#L3231C1-L3235C4

@bcardosolopes bcardosolopes merged commit 55a2236 into llvm:main Mar 25, 2025
9 checks passed
@el-ev el-ev deleted the pragma_comment branch March 27, 2025 12:05
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
…#1521)

- Part of llvm#70

This is not very useful now, as both branches of interest are NYI. I
will try to implement the real features in subsequent patches.
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.

2 participants