DiggerLin added inline comments.
================ Comment at: clang/lib/CodeGen/BackendUtil.cpp:520 Options.DataSections = CodeGenOpts.DataSections; - Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility; Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames; ---------------- hubert.reinterpretcast wrote: > DiggerLin wrote: > > sfertile wrote: > > > DiggerLin wrote: > > > > sfertile wrote: > > > > > DiggerLin wrote: > > > > > > jasonliu wrote: > > > > > > > DiggerLin wrote: > > > > > > > > DiggerLin wrote: > > > > > > > > > jasonliu wrote: > > > > > > > > > > DiggerLin wrote: > > > > > > > > > > > jasonliu wrote: > > > > > > > > > > > > DiggerLin wrote: > > > > > > > > > > > > > jasonliu wrote: > > > > > > > > > > > > > > Instead of just removing this line, should this get > > > > > > > > > > > > > > replaced with the new LangOpts option? > > > > > > > > > > > > > I do not think we need a CodeGenOp of > > > > > > > > > > > > > ignore-xcoff-visibility in clang, we only need the > > > > > > > > > > > > > LangOpt of the ignore-xcoff-visilbity to control > > > > > > > > > > > > > whether we will generate the visibility in the IR, > > > > > > > > > > > > > when the LangOpt of ignore-xcoff-visibility do not > > > > > > > > > > > > > generate the visibility attribute of GV in the IR. it > > > > > > > > > > > > > do not need CodeGenOp of ignore-xcoff-visibility any > > > > > > > > > > > > > more for the clang . > > > > > > > > > > > > > > > > > > > > > > > > > > we have still CodeGen ignore-xcoff-visibility op in > > > > > > > > > > > > > llc. > > > > > > > > > > > > We removed the visibility from IR level with this > > > > > > > > > > > > patch. But there is also visibility settings coming > > > > > > > > > > > > from CodeGen part of clang, which needs to get ignore > > > > > > > > > > > > when we are doing the code gen in llc. So I think you > > > > > > > > > > > > still need to set the options correct for llc. > > > > > > > > > > > yes we have the set the options correct for llc in the > > > > > > > > > > > code. > > > > > > > > > > > > > > > > > > > > > > in the source file llvm/lib/CodeGen/CommandFlags.cpp, we > > > > > > > > > > > have (in the patch https://reviews.llvm.org/D87451 add > > > > > > > > > > > new option -mignore-xcoff-visibility) , the function > > > > > > > > > > > TargetOptions > > > > > > > > > > > codegen::InitTargetOptionsFromCodeGenFlags() { > > > > > > > > > > > .... > > > > > > > > > > > Options.IgnoreXCOFFVisibility = > > > > > > > > > > > getIgnoreXCOFFVisibility(); > > > > > > > > > > > ...} > > > > > > > > > > > > > > > > > > > > > What I'm saying is... > > > > > > > > > > I think we need a line like this: > > > > > > > > > > `Options.IgnoreXCOFFVisibility = > > > > > > > > > > LangOpts.IgnoreXCOFFVisibility;` > > > > > > > > > > so that when you invoke clang, backend would get the > > > > > > > > > > correct setting as well. > > > > > > > > > I do not think so, from the clang FE, we do not generated the > > > > > > > > > visibility in the IR. so there is no need these line. > > > > > > > > or we can say that because we do not set the hidden visibility > > > > > > > > into the GlobalValue , so we do not need the > > > > > > > > Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility; > > > > > > > I think I mentioned this before, we could have extra visibility > > > > > > > settings in clang/lib/CodeGen that's not depending on the > > > > > > > existing visibility setting in the IR. (You could search for > > > > > > > `setVisibility` there.) That was the reason we did it in llc > > > > > > > first. > > > > > > I will add Options.IgnoreXCOFFVisibility = > > > > > > LangOpts.IgnoreXCOFFVisibility; here. > > > > > > I think I mentioned this before, we could have extra visibility > > > > > > settings in clang/lib/CodeGen that's not depending on the existing > > > > > > visibility setting in the IR. (You could search for setVisibility > > > > > > there.) That was the reason we did it in llc first. > > > > > > > > > > A lot of these are in places we wouldn't encounter with AIX, like > > > > > for Objective-C code gen. But are others like [[ > > > > > https://github.com/llvm/llvm-project/blob/b03ea054db1bcf9452b3a70e21d3372b6e58759a/clang/lib/CodeGen/ItaniumCXXABI.cpp#L2507 > > > > > | this]] an issue? Should they be addressed in this patch? > > > > after I added the Options.IgnoreXCOFFVisibility = > > > > LangOpts.IgnoreXCOFFVisibility , even there is > > > > GV->setVisibility(llvm::GlobalValue::HiddenVisibility); it do not > > > > effect our output. > > > > > > > > there is following code in the function void > > > > PPCAIXAsmPrinter::emitLinkage(const GlobalValue *GV, > > > > MCSymbol *GVSym) const > > > > { > > > > ..... > > > > if (!TM.getIgnoreXCOFFVisibility()) { > > > > switch (GV->getVisibility()) { > > > > > > > > // TODO: "exported" and "internal" Visibility needs to go here. > > > > case GlobalValue::DefaultVisibility: > > > > break; > > > > case GlobalValue::HiddenVisibility: > > > > VisibilityAttr = MAI->getHiddenVisibilityAttr(); > > > > break; > > > > case GlobalValue::ProtectedVisibility: > > > > VisibilityAttr = MAI->getProtectedVisibilityAttr(); > > > > break; > > > > } > > > > } > > > > > > > > ... > > > > } > > > > it do not effect our output. > > > It can if we set the GlobalValue to be dso_local though ... that is the > > > whole point of this patch. > > can you give me the example of when the GlobalValue will be set to > > dso_local when there is a visibility been ignore in AIX? > Just noting that setting dso_local (for reasons other than visibility) when > visibility is being ignored on AIX is something that should be possible > (e.g., for Extended Operations). 1. discussed with sean offline, the we do not call the emitGlobalDtorWithCXAAtExit() in AIX. so we do not have the problem for the "__dso_handle" 2. for the visibility, if we ignore the visibility , we do not set it to default visibility( the visibility will be set to unspecific ) when we output assembly or xcoff objectfile by llc . And for the dso file, the unspecific symbol will not be exported , and the exported symbols of dso will be depend on the export list file. So I do not think there is problem for the compiler generated symbol when we ignore visibility. for example. when there is compiler generated symbol _ _gc_log in a asm file . .globl _ _gc_log when the aix as generated the xcoff object file, the visibility of the symbol will be unspecific , not default. only the following asm has default visibility. .globl _ _gc_log, exported Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89986/new/ https://reviews.llvm.org/D89986 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
