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: > 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? ================ Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:1 // REQUIRES: powerpc-registered-target ---------------- You shouldn't need this requires anymore. ================ Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:67 +// NOVISIBILITY-IR: define void @_Z7prambarv() + ---------------- minor nit: Remove the blank line. ================ Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:1 +// REQUIRES: powerpc-registered-target + ---------------- Shouldn't need this requires either. ================ Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:4 +// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s | \ +// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s + ---------------- Can we drop the COMMON-IR part of the test? I don't think it adds much value but clutters the run lines. ================ Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:13 + +// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -fvisibility-inlines-hidden \ +// RUN: -fvisibility default -emit-llvm -o - -x c++ %s | \ ---------------- There is trailing white space at the end of this line. 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