search

llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
307 stars 84 forks source link

[CIR][CIRGen] Add dsolocal attribute to GlobalOp and FuncOp #686

Closed ghehg closed 1 week ago

ghehg commented 3 weeks ago

as title. In this PR

  1. make setDSOLocal an interface function.
  2. implemented shouldAssumeDSOLocal function in CIRGenModule, using the same skeleton as shouldAssumeDSOLocal in OG's CodeGenModule.cpp.
  3. added call sites of setDSOLocal within CIRGenModule, like what's in OG's CodeGenModule.
  4. fixed printing format
  5. LLVM lowering
  6. keep CIRGenModule::setDSOLocal(mlir::Operation *Op) wrapper at call sites, so if we make changes to interface, we don't have to touch call sites since there are many.

We don't have LLVM test for this PR yet, and it will be addressed by the next PR,: TODO in the next PR:

  1. Implement setNonAliasAttributes in CIRGenModule.cpp, which should be called by CIRGenModule::buildGlobalFunctionDefinition. That way, we will set dso_local correctly for all func ops who have defs in the module. That way we should have LLVM test case in this next PR. detailed explanation below:

Since LLVM asm printer omits dso_local in isImplicitDSOLocal(), and all we cover so far in CIR all fall into this category, we're not able to have a LLVM test. However, the case isDeclarationForLinker() should have a lot of test examples as all func defs should have dso_local, We don't have it CIR is because A to-do in our CG. When OG is building func def, after code is generated, it will call setDSOLocal again via setNonAliasAttributes—>SetCommonAttributes—>setGVProperties. The key difference is now GV is not declaration anymore. so satisfies the test if (!GV->isDeclarationForLinker()) return true; https://github.com/llvm/clangir/blob/f78f9a55e7cd6b9e350556e35097616676cf1f3e/clang/lib/CodeGen/CodeGenModule.cpp#L5864 But our CG missed this step of calling setNonAliasAttributes so it won’t give setDSOLocal another chance to get it right https://github.com/llvm/clangir/blob/c28908396a3ba7bda6345907233e4f5c4e53a33e/clang/lib/CIR/CodeGen/CIRGenModule.cpp#L496

TODO in the next next PR

  1. add call to setDSOLocal in other parts of CG other than CIRGenModule.
  2. implement DefaultVisibility check, didn't do in this PR as LLVM::DefaultVisibility has no direct counterpart in MLIR::. Therefore, it takes care examination of cases to see what is the best emulation of hasDefaultVisibility in MLIR/CIR context as far as dsolocal is concerned.

TODO in future other than DefaultVisibility check, we didn't implement canBenefitFromLocalAlias as it depends on other missing features like setComDat.

There is a lot of cases we need to cover, so this is just the first step!

bcardosolopes commented 2 weeks ago

Titles shouldn't be too long, just edited as an example, you can always add more info in the description.