-
Notifications
You must be signed in to change notification settings - Fork 182
[CIR] Add support for FileScopeAsm #1995
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
base: main
Are you sure you want to change the base?
Conversation
| appendModuleInlineAsm(D->getAsmString()); | ||
| } | ||
|
|
||
| void CIRGenModule::finalizeModuleAttributes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name might be too generic. Should I rename it to something more specific?
| // Handle module-level inline assembly | ||
| if (auto moduleAsmAttr = | ||
| mlir::dyn_cast<cir::ModuleAsmAttr>(attribute.getValue())) { | ||
| mlir::ArrayAttr asmStrings = moduleAsmAttr.getAsmStrings(); | ||
| for (mlir::Attribute attr : asmStrings) { | ||
| if (auto strAttr = mlir::dyn_cast<mlir::StringAttr>(attr)) { | ||
| llvmModule->appendModuleInlineAsm(strAttr.getValue()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to place the lowering here or is there another place you guys prefer?
2ff14fa to
0b01e1c
Compare
0b01e1c to
e01ecbf
Compare
bcardosolopes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving this, comments inline
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // FileScopeAsm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> CIR_ModuleAsmAttr?
| break; | ||
| } | ||
| case Decl::FileScopeAsm: | ||
| buildFileScopeAsm(cast<FileScopeAsmDecl>(decl)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow OG skeleton:
case Decl::FileScopeAsm:
// File-scope asm is ignored during device-side CUDA compilation.
if (langOpts.CUDA && langOpts.CUDAIsDevice)
break;
// File-scope asm is ignored during device-side OpenMP compilation.
if (langOpts.OpenMPIsTargetDevice)
break;
// File-scope asm is ignored during device-side SYCL compilation.
if (langOpts.SYCLIsDevice)
break;
buildFileScopeAsm(cast<FileScopeAsmDecl>(decl));
break;
}
| break; | ||
| } | ||
| case Decl::FileScopeAsm: | ||
| buildFileScopeAsm(cast<FileScopeAsmDecl>(decl)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just call moduleAsm.push_back(Asm.str()); directly here, the two levels of indirection are not providing any benefits right now
| } | ||
|
|
||
| // Finalize module attributes (including inline assembly) | ||
| finalizeModuleAttributes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OG doesn't do this right here, why are we? There's already other model level attributes being handled, the content of this function should be there
| // LLVM: module asm ".section .rodata" | ||
| // LLVM: module asm ".string {{\\22}}hello{{\\22}}" | ||
|
|
||
| // LLVM: define{{.*}} i32 @foo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look for OGCG in other tests and add the similar here.
No description provided.