sfertile 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: > > > 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. ================ Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:1 +// REQUIRES: powerpc-registered-target + ---------------- DiggerLin wrote: > 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 Look at the run steps in the test when that was added. eg: ``` // RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S %s |\ // RUN: FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s ``` You are producing **assembly** output for a powerpc target. If you don't compile the powerpc backend then you will get a message along the lines of `error: unable to create target: 'No available targets are compatible with triple "powerpc-unknown-aix"'` But now you have cleaned up the test to only produce IR. That doesn't require a powerpc code generator to run the test, so we can remove the dependency (in both this test and the other). 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
