[PATCH] D87451: add new option ignore-xcoff-visibility

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 296267.
DiggerLin marked 3 inline comments as done.
DiggerLin added a comment.

address comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/Driver/ignore-xcoff-visibility.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll

Index: llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll
@@ -0,0 +1,48 @@
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec  < %s | \
+; RUN:   FileCheck --check-prefix=VISIBILITY-ASM %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \
+; RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefix=VISIBILITY-ASM %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \
+; RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+
+@foo_p = global void ()* @zoo_extern_h, align 4
+@b = protected global i32 0, align 4
+
+define hidden void @foo_h(i32* %p) {
+entry:
+  %p.addr = alloca i32*, align 4
+  store i32* %p, i32** %p.addr, align 4
+  %0 = load i32*, i32** %p.addr, align 4
+  %1 = load i32, i32* %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, i32* %0, align 4
+  ret void
+}
+
+declare hidden void @zoo_extern_h()
+
+define protected void @bar() {
+entry:
+  call void @foo_h(i32* @b)
+  %0 = load void ()*, void ()** @foo_p, align 4
+  call void %0()
+  ret void
+}
+
+; VISIBILITY-ASM: .globl  foo_h[DS],hidden
+; VISIBILITY-ASM: .globl  .foo_h,hidden
+; VISIBILITY-ASM: .globl  bar[DS],protected
+; VISIBILITY-ASM: .globl  .bar,protected
+; VISIBILITY-ASM: .globl  b,protected
+
+; IGNOREVISIBILITY-ASM: .globl  foo_h[DS]
+; IGNOREVISIBILITY-ASM: .globl  .foo_h
+; IGNOREVISIBILITY-ASM: .globl  bar[DS]
+; IGNOREVISIBILITY-ASM: .globl  .bar
+; IGNOREVISIBILITY-ASM: .globl  b
Index: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
===
--- llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -1702,17 +1702,19 @@
   assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid.");
 
   MCSymbolAttr VisibilityAttr = MCSA_Invalid;
-  switch (GV->getVisibility()) {
+  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;
+// 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;
+}
   }
 
   OutStreamer->emitXCOFFSymbolLinkageWithVisibility(GVSym, LinkageAttr,
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -74,6 +74,7 @@
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
+CGOPT(bool, IgnoreXCOFFVisibility)
 CGOPT(std::string, BBSections)
 CGOPT(unsigned, TLSSize)
 CGOPT(bool, EmulatedTLS)
@@ -333,6 +334,13 @@
   cl::init(false));
   CGBINDOPT(FunctionSections);
 
+  static cl::opt IgnoreXCOFFVisibility(
+  "ignore-xcoff-visibility",
+  cl::desc("Not emit the visibility attribute for asm in AIX OS or give "
+   "all symbols 'unspecified' visibility in xcoff object file"),
+  cl::init(false));
+  CGBINDOPT(IgnoreXCOFFVisibility);
+
   static cl::opt BBSections(
   "basic-block-sections",
   cl::desc("Emit basic blocks into separate 

[PATCH] D87451: add new option ignore-xcoff-visibility

[Digger via Phabricator via cfe-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2622
+
+Do not emit any visibility attribute for asm on AIX or give all symbols 
'unspecified' visibility in xcoff object file(XCOFF only)
+

daltenty wrote:
> daltenty wrote:
> > nit: add a space before parens
> I don't think the object file writing case was handled yet? This makes it 
> sound like it is.
yes, we do not implement the visibility of the object file in the 
XCoffObjectWriter.cpp



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5242
 
+  if (const Arg *A = Args.getLastArg(options::OPT_mignore_xcoff_visibility)) {
+if (Triple.isOSAIX())

daltenty wrote:
> Use `Args.hasFlag` instead, since this option doesn't have a value we need to 
> check.
we need to get the value here 
 D.Diag(diag::err_drv_unsupported_opt_for_target)
  << A->getAsString(Args) << TripleStr;



Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:72
+
+// ERROR: unsupported option '-mignore-xcoff-visibility' for target 
'powerpc-unknown-linux'
+

daltenty wrote:
> This isn't being checked anymore, also probably belongs in the other file
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87451: add new option ignore-xcoff-visibility

[David Tenty via Phabricator via cfe-commits]

daltenty added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2622
+
+Do not emit any visibility attribute for asm on AIX or give all symbols 
'unspecified' visibility in xcoff object file(XCOFF only)
+

nit: add a space before parens



Comment at: clang/docs/ClangCommandLineReference.rst:2622
+
+Do not emit any visibility attribute for asm on AIX or give all symbols 
'unspecified' visibility in xcoff object file(XCOFF only)
+

daltenty wrote:
> nit: add a space before parens
I don't think the object file writing case was handled yet? This makes it sound 
like it is.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5242
 
+  if (const Arg *A = Args.getLastArg(options::OPT_mignore_xcoff_visibility)) {
+if (Triple.isOSAIX())

Use `Args.hasFlag` instead, since this option doesn't have a value we need to 
check.



Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:72
+
+// ERROR: unsupported option '-mignore-xcoff-visibility' for target 
'powerpc-unknown-linux'
+

This isn't being checked anymore, also probably belongs in the other file



Comment at: clang/test/Driver/ignore-xcoff-visibility.cpp:2
+// RUN: %clang -### -target powerpc-unknown-aix  -mignore-xcoff-visibility -S 
%s 2> %t.log
+// RUN: FileCheck -check-prefix=CHECK %s < %t.log
+// CHECK: "-mignore-xcoff-visibility"

We should check the diagnostic here



Comment at: clang/test/Driver/ignore-xcoff-visibility.cpp:3
+// RUN: FileCheck -check-prefix=CHECK %s < %t.log
+// CHECK: "-mignore-xcoff-visibility"

nit: We should constrain this to be following the cc1 invocation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87451: add new option ignore-xcoff-visibility

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 295838.
DiggerLin marked an inline comment as done.
DiggerLin added a comment.

address comment and merger the patch https://reviews.llvm.org/D88234 into the 
patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/Driver/ignore-xcoff-visibility.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll

Index: llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll
@@ -0,0 +1,48 @@
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec  < %s | \
+; RUN:   FileCheck --check-prefix=VISIBILITY-ASM %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \
+; RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefix=VISIBILITY-ASM %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \
+; RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+
+@foo_p = global void ()* @zoo_extern_h, align 4
+@b = protected global i32 0, align 4
+
+define hidden void @foo_h(i32* %p) {
+entry:
+  %p.addr = alloca i32*, align 4
+  store i32* %p, i32** %p.addr, align 4
+  %0 = load i32*, i32** %p.addr, align 4
+  %1 = load i32, i32* %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, i32* %0, align 4
+  ret void
+}
+
+declare hidden void @zoo_extern_h()
+
+define protected void @bar() {
+entry:
+  call void @foo_h(i32* @b)
+  %0 = load void ()*, void ()** @foo_p, align 4
+  call void %0()
+  ret void
+}
+
+; VISIBILITY-ASM: .globl  foo_h[DS],hidden
+; VISIBILITY-ASM: .globl  .foo_h,hidden
+; VISIBILITY-ASM: .globl  bar[DS],protected
+; VISIBILITY-ASM: .globl  .bar,protected
+; VISIBILITY-ASM: .globl  b,protected
+
+; IGNOREVISIBILITY-ASM: .globl  foo_h[DS]
+; IGNOREVISIBILITY-ASM: .globl  .foo_h
+; IGNOREVISIBILITY-ASM: .globl  bar[DS]
+; IGNOREVISIBILITY-ASM: .globl  .bar
+; IGNOREVISIBILITY-ASM: .globl  b
Index: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
===
--- llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -1702,17 +1702,19 @@
   assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid.");
 
   MCSymbolAttr VisibilityAttr = MCSA_Invalid;
-  switch (GV->getVisibility()) {
+  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;
+// 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;
+}
   }
 
   OutStreamer->emitXCOFFSymbolLinkageWithVisibility(GVSym, LinkageAttr,
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -74,6 +74,7 @@
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
+CGOPT(bool, IgnoreXCOFFVisibility)
 CGOPT(std::string, BBSections)
 CGOPT(unsigned, TLSSize)
 CGOPT(bool, EmulatedTLS)
@@ -333,6 +334,13 @@
   cl::init(false));
   CGBINDOPT(FunctionSections);
 
+  static cl::opt IgnoreXCOFFVisibility(
+  "ignore-xcoff-visibility",
+  cl::desc("Not emit the visibility attribute for asm in AIX OS or give "
+   "all symbols 'unspecified' visibility in xcoff object file"),
+  cl::init(false));
+  CGBINDOPT(IgnoreXCOFFVisibility);
+
   static cl::opt BBSections(
   

[PATCH] D87451: add new option ignore-xcoff-visibility

[Digger via Phabricator via cfe-commits]

DiggerLin marked 5 inline comments as done.
DiggerLin added inline comments.



Comment at: clang/test/CodeGen/aix-no-xcoff-visibility.cpp:75
+
+// VISIBILITY-IR:@b = protected global i32 0
+// VISIBILITY-IR:@pramb = hidden global i32 0

jasonliu wrote:
> Not sure if the IR check is really necessary, since we haven't made any IR 
> change here. It's going to be all the same with or without the new -m option. 
IR output will not change in the new option. I think we need to check whether 
the IR be changed in the new option 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits