DiggerLin marked 9 inline comments 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: > > 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; } } ... } ================ Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:1 // REQUIRES: powerpc-registered-target ---------------- sfertile wrote: > You shouldn't need this requires anymore. please see the https://reviews.llvm.org/rGa15bd0bfc20c2b2955c59450a67b6e8efe89c708 ================ Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:1 +// REQUIRES: powerpc-registered-target + ---------------- jasonliu wrote: > sfertile wrote: > > Shouldn't need this requires either. > Do you need this line? there is a comment in https://reviews.llvm.org/D87451 " Hello. I added a power-pc REQUIRES clause to the new clang test here in a15bd0bfc20c2b2955c59450a67b6e8efe89c708. Hope that looks OK." please see the https://reviews.llvm.org/rGa15bd0bfc20c2b2955c59450a67b6e8efe89c708 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
