DiggerLin marked an inline comment as done. DiggerLin added inline comments.
================ Comment at: clang/lib/CodeGen/BackendUtil.cpp:520 Options.DataSections = CodeGenOpts.DataSections; - Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility; Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames; ---------------- 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? 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
