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

[Digger Lin via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG46d4d1fea401: [AIX] do not emit visibility attribute into IR 
when there is -mignore-xcoff… (authored by DiggerLin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,26 +1,19 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -round-trip-args -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
 __attribute__((visibility("hidden"))) void foo_h(int *p) {
@@ -70,28 +63,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 329316.
DiggerLin added a comment.

slight change , move to

  if (Opts.IgnoreXCOFFVisibility)
GenerateArg(Args, OPT_mignore_xcoff_visibility, SA);

to position as comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,26 +1,19 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -round-trip-args -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
 __attribute__((visibility("hidden"))) void foo_h(int *p) {
@@ -70,28 +63,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D89986#2612021 , @DiggerLin wrote:

> Gentle ping . @jansvoboda11 .  Do you have any comment on it the last update 
> on  "added -round-trip-args functionality for the lang opt 
> "-mignore-xcoff-visibility" ?

I'd slightly prefer the code for `OPT_mignore_xcoff_visibility` argument 
generation to be in a similar place the parsing code is (between 
`OPT_fgnuc_version_EQ ` and `OPT_ftrapv`), but otherwise this change looks good 
to me.


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

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin added a comment.

Gentle ping . @jansvoboda11 .  Do you have any comment on it the last update on 
 "added -round-trip-args functionality for the lang opt 
"-mignore-xcoff-visibility" ?


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

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

[Lei Huang via Phabricator via cfe-commits]

lei added a comment.

Instead of waiting a day or two, can you please directly ping reviewers who had 
concerns related to the round-trip-args behaviour to get feedback?


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

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

[Sean Fertile via Phabricator via cfe-commits]

sfertile accepted this revision.
sfertile added a subscriber: Xiangling_L.
sfertile added a comment.

> discussed with sean offline, the we do not call the 
> emitGlobalDtorWithCXAAtExit() in AIX. so we do not have the problem for the 
> "__dso_handle"

I initially had concerns with the places clang introduces non-default 
visibility in codegen. The few cases I was worried we would hit were related to 
static init , but that was before @Xiangling_L static init related work.   
After those changes I have no other concerns. I would wait a day or two to 
ensure that there are no more comments  related to the round-trip-args 
behaviour but otherwise LGTM.


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

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

[Digger Lin via Phabricator via cfe-commits]

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

added extra comment back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,26 +1,19 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -round-trip-args -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
 __attribute__((visibility("hidden"))) void foo_h(int *p) {
@@ -70,28 +63,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// 

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin marked an inline comment as done.
DiggerLin added a comment.






Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3630
+  // no -fvisibility=* option.
+  if (T.isOSAIX() && (Args.hasArg(OPT_mignore_xcoff_visibility) ||
+  !Args.hasArg(OPT_fvisibility)))

daltenty wrote:
> No sure if we intended to move this block? Regardless, we should probably 
> preserve the extra comments that got added.
in original patch https://reviews.llvm.org/D87451 , the 
-mignore-xcoff-visibility is CodeGenOpt, in the new patch, we change the option 
"-mignore-xcoff-visibility" as LangOpt. 
I will add the extra comment back.


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

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 328769.
DiggerLin added a comment.

added -round-trip-args functionality for the lang opt 
"-mignore-xcoff-visibility"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,26 +1,19 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -round-trip-args -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -round-trip-args -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
 
 __attribute__((visibility("hidden"))) void foo_h(int *p) {
@@ -70,28 +63,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  

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

[David Tenty via Phabricator via cfe-commits]

daltenty added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3630
+  // no -fvisibility=* option.
+  if (T.isOSAIX() && (Args.hasArg(OPT_mignore_xcoff_visibility) ||
+  !Args.hasArg(OPT_fvisibility)))

No sure if we intended to move this block? Regardless, we should probably 
preserve the extra comments that got added.


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

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 328632.
DiggerLin added a comment.

rebase code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,8 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +54,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z3barv
-// IGNOREVISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS]
-// IGNOREVISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv
-// IGNOREVISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS]
-// 

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

FYI, this might need rebasing on top of D97552 
.


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

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 327898.
DiggerLin added a comment.

modify a test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,8 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +54,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z3barv
-// IGNOREVISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS]
-// IGNOREVISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv
-// IGNOREVISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS]
-// 

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 327804.
DiggerLin added a comment.

rebased the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,8 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +54,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* {{[^,]*}} %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z3barv
-// IGNOREVISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS]
-// IGNOREVISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv
-// IGNOREVISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS]
-// 

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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin added a comment.
Herald added a subscriber: jansvoboda11.

Gentle ping @sfertile , any new comment on the patch ?


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

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

[Digger via Phabricator via cfe-commits]

DiggerLin added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > 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 

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast 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:
> > > 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 

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

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 301029.
DiggerLin added a comment.

remove REQUIRES: powerpc-registered-target  from test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,8 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +54,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z3barv
-// IGNOREVISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS]
-// IGNOREVISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv
-// IGNOREVISIBILITY-ASM: .weak   

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

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 301020.
DiggerLin marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,39 @@
+// REQUIRES: powerpc-registered-target
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
+// RUN:-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,10 @@
 // REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +56,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z3barv
-// IGNOREVISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS]
-// IGNOREVISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv
-// IGNOREVISIBILITY-ASM: .weak   

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

[Digger via Phabricator via cfe-commits]

DiggerLin marked an inline comment 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:
> > 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 

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

[Sean Fertile via Phabricator via cfe-commits]

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:

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

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 300972.
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/D89986/new/

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,39 @@
+// REQUIRES: powerpc-registered-target
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+   -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -emit-llvm \
+// RUN:-fvisibility-inlines-hidden -fvisibility default -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,10 @@
 // REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +56,11 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z3barv
-// IGNOREVISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS]
-// IGNOREVISIBILITY-ASM: .weak   

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

[Digger via Phabricator via cfe-commits]

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

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

[Sean Fertile via Phabricator via cfe-commits]

sfertile added inline comments.



Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:6
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large 
-fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s

Can you break up this RUN step similar to how you formatted the following one.


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

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

[Sean Fertile via Phabricator via cfe-commits]

sfertile added a comment.

Sorry, missed a comment.


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

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

[Sean Fertile via Phabricator via cfe-commits]

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

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

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu added a comment.
This revision is now accepted and ready to land.

LGTM with minor nit.




Comment at: clang/lib/AST/Decl.cpp:1481
 LinkageInfo LinkageComputer::getDeclLinkageAndVisibility(const NamedDecl *D) {
-  return getLVForDecl(D,
-  LVComputationKind(usesTypeVisibility(D)
-? NamedDecl::VisibilityForType
-: NamedDecl::VisibilityForValue));
+  NamedDecl::ExplicitVisibilityKind EK = usesTypeVisibility(D)
+ ? NamedDecl::VisibilityForType

`clang-format` this please.
This line seems to exceed 80 columns. 



Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:1
+// REQUIRES: powerpc-registered-target
+

Do you need 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

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

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 300766.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,43 @@
+// REQUIRES: powerpc-registered-target
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -fvisibility-inlines-hidden \ 
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+// COMMON-IR-NEXT:  entry:
+// COMMON-IR-NEXT:store i32 55, i32* @x, align 4
+// COMMON-IR-NEXT:ret void
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
+// COMMON-IR-NEXT:  entry:
+// COMMON-IR-NEXT:store i32 55, i32* @x, align 4
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,10 @@
 // REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +56,12 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// IGNOREVISIBILITY-ASM: .globl  _Z3barv[DS]
-// IGNOREVISIBILITY-ASM: 

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

[Digger via Phabricator via cfe-commits]

DiggerLin added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

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.


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

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

[Jason Liu via Phabricator via cfe-commits]

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


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

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

[Digger via Phabricator via cfe-commits]

DiggerLin marked an inline comment 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;

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;


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

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

[Digger via Phabricator via cfe-commits]

DiggerLin marked 2 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;

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.


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

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

[Jason Liu via Phabricator via cfe-commits]

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

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

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

[Digger via Phabricator via cfe-commits]

DiggerLin marked 2 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;

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(); 
...}



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

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

[Jason Liu via Phabricator via cfe-commits]

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


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

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

[Digger via Phabricator via cfe-commits]

DiggerLin marked an inline comment 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;

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.


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

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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

Instead of just removing this line, should this get replaced with the new 
LangOpts option?


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

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

[Digger via Phabricator via cfe-commits]

DiggerLin updated this revision to Diff 300421.
DiggerLin added a comment.

delete asm test case from clang/test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/LangOptions.def
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
  clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Index: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
@@ -0,0 +1,43 @@
+// REQUIRES: powerpc-registered-target
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden \
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,VISIBILITY-IR %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -mignore-xcoff-visibility -fvisibility-inlines-hidden \ 
+// RUN:-fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,NOVISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+
+#pragma GCC visibility push(hidden)
+__attribute__((__noinline__)) inline void foo() {
+  x = 55;
+}
+#pragma GCC visibility pop
+
+int bar() {
+  f();
+  foo();
+  return x;
+}
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z1fv()
+// COMMON-IR-NEXT:  entry:
+// COMMON-IR-NEXT:store i32 55, i32* @x, align 4
+// COMMON-IR-NEXT:ret void
+
+// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
+// NOVISIBILITY-IR:   define linkonce_odr void @_Z3foov()
+// COMMON-IR-NEXT:  entry:
+// COMMON-IR-NEXT:store i32 55, i32* @x, align 4
Index: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
===
--- clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
+++ clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
@@ -1,24 +1,10 @@
 // REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
-// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s
 
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=IGNOREVISIBILITY-ASM %s
-
-// RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -o - -x c++ -S %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-ASM %s
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -mignore-xcoff-visibility -fvisibility default -emit-llvm -o - -x c++ %s  | \
-// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
 
 // RUN: %clang_cc1 -triple powerpc-unknown-aix -fvisibility default -emit-llvm -o - -x c++ %s  | \
 // RUN: FileCheck -check-prefix=VISIBILITY-IR %s
@@ -70,28 +56,12 @@
 // VISIBILITY-IR:define weak_odr protected i32 @_ZN5basicIiE7getdataEv(%class.basic* %this)
 // VISIBILITY-IR:define hidden void @_Z7prambarv()
 
-// VISIBILITY-ASM: .globl  _Z5foo_hPi[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z5foo_hPi,hidden
-// VISIBILITY-ASM: .globl  _Z3barv[DS],protected
-// VISIBILITY-ASM: .globl  ._Z3barv,protected
-// VISIBILITY-ASM: .weak   _ZNK9TestClass5valueEv[DS],hidden
-// VISIBILITY-ASM: .weak   ._ZNK9TestClass5valueEv,hidden
-// VISIBILITY-ASM: .weak   _ZN5basicIiE7getdataEv[DS],protected
-// VISIBILITY-ASM: .weak   ._ZN5basicIiE7getdataEv,protected
-// VISIBILITY-ASM: .globl  _Z7prambarv[DS],hidden
-// VISIBILITY-ASM: .globl  ._Z7prambarv,hidden
-// VISIBILITY-ASM: .globl  b,protected
-// VISIBILITY-ASM: .globl  pramb,hidden
-
-// IGNOREVISIBILITY-ASM: .globl  _Z5foo_hPi[DS]
-// IGNOREVISIBILITY-ASM: .globl  ._Z5foo_hPi
-// 

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:30-34
+// COMMON-ASM: mflr 0
+// COMMON-ASM-NEXT:stw 0, 8(1)
+// COMMON-ASM-NEXT:stwu 1, -64(1)
+// COMMON-ASM-NEXT:bl ._Z1fv
+// NOP-ASM-NEXT:   nop

DiggerLin wrote:
> nemanjai wrote:
> > rsmith wrote:
> > > Generally we strongly prefer for frontend tests to test only the 
> > > frontend; in this case, that means testing only the IR that Clang is 
> > > producing and not the assembly that comes out of LLVM for that IR. This 
> > > should also remove the need to require PPC as a registered target.
> > +1
> > The asm test can go into `llvm/test/CodeGen/PowerPC`.
> If I put the ASM test as separate test case in the llvm/test/CodeGen/PowerPC 
> .it will have error as :
> 
> ine 1: %clang_cc1: command not found
That's precisely the point.
Clang codegen tests should only test clang IR generation.
They should not invoke -O1/etc optimization pipelines.
LLVM transform tests should only test IR->IR transforms, they should not invoke 
clang/llc
LLVM codegen tests should only test IR->ASM codegen, they should not invoke 
clang


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