[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

Mon Oct 26 11:35:46 PDT 2020

jasonliu 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:
> > > 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. 

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89986/new/

https://reviews.llvm.org/D89986