hubert.reinterpretcast added inline comments.
================ Comment at: clang/lib/CodeGen/BackendUtil.cpp:520 Options.DataSections = CodeGenOpts.DataSections; - Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility; Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames; ---------------- 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). 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
