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

[DiggerLin]

Summary

in the patch https://reviews.llvm.org/D87451 "add new option -mignore-xcoff-visibility"
we did as "The option -mignore-xcoff-visibility has no effect on visibility attribute when compile with -emit-llvm option to generated LLVM IR."

in these patch we let -mignore-xcoff-visibility effect on generating IR too. the new feature only work on AIX OS

[hubert.reinterpretcast]

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

20

There's no testing for the interaction with -mignore-xcoff-visibility.

29

The general issue is that the nop instruction is being omitted even when the visibility information is not generated into the object file. This is also a problem when the visibility is applied in the source code:

int x = 66;
#pragma GCC visibility push(hidden)
__attribute__((__noinline__)) inline void f() {
  x = 55;
}
#pragma GCC visibility pop
int bar() {
  f();
  return x;
}

[rsmith]

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

30–34

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.

[nemanjai]

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

30–34

+1
The asm test can go into llvm/test/CodeGen/PowerPC.

[DiggerLin]

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

30–34

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

[DiggerLin]

address comment

[lebedev.ri]

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

30–34

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

[DiggerLin]

delete asm test case from clang/test

[jasonliu]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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.

[jasonliu]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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.

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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

[jasonliu]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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.

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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.

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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;

[jasonliu]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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.

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

I will add Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility; here.

[jasonliu]

LGTM with minor nit.

clang/lib/AST/Decl.cpp

1481 ↗

(On Diff #300766)

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

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

2

Do you need this line?

[sfertile]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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 this an issue? Should they be addressed in this patch?

clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp

1 ↗

(On Diff #300766)

You shouldn't need this requires anymore.

67 ↗

(On Diff #300766)

minor nit: Remove the blank line.

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

2

Shouldn't need this requires either.

5

Can we drop the COMMON-IR part of the test? I don't think it adds much value but clutters the run lines.

14

There is trailing white space at the end of this line.

[sfertile]

Sorry, missed a comment.

[sfertile]

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

7

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

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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;
  }
}

...
}

clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp

1 ↗

(On Diff #300766)

please see the https://reviews.llvm.org/rGa15bd0bfc20c2b2955c59450a67b6e8efe89c708

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

2

there is a comment in https://reviews.llvm.org/D87451 " Hello. I added a power-pc REQUIRES clause to the new clang test here in a15bd0bfc20c2b2955c59450a67b6e8efe89c708. Hope that looks OK."

please see the
https://reviews.llvm.org/rGa15bd0bfc20c2b2955c59450a67b6e8efe89c708

[DiggerLin]

address comment

[sfertile]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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.

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

2

Look at the run steps in the test when that was added. eg:

// RUN: %clang_cc1 -triple powerpc-unknown-aix -o - -x c++ -S  %s  |\
// RUN:   FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s

You are producing assembly output for a powerpc target. If you don't compile the powerpc backend then you will get a message along the lines of
error: unable to create target: 'No available targets are compatible with triple "powerpc-unknown-aix"'

But now you have cleaned up the test to only produce IR. That doesn't require a powerpc code generator to run the test, so we can remove the dependency (in both this test and the other).

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

can you give me the example of when the GlobalValue will be set to dso_local when there is a visibility been ignore in AIX?

[DiggerLin]

remove REQUIRES: powerpc-registered-target from test case

[hubert.reinterpretcast]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

Just noting that setting dso_local (for reasons other than visibility) when visibility is being ignored on AIX is something that should be possible (e.g., for Extended Operations).

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520 ↗

(On Diff #300421)

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

2. for the visibility, if we ignore the visibility , we do not set it to default visibility( the visibility will be set to unspecific ) when we output assembly or xcoff objectfile by llc . And for the dso file, the unspecific symbol will not be exported , and the exported symbols of dso will be depend on the export list file. So I do not think there is problem for the compiler generated symbol when we ignore visibility.

for example. when there is compiler generated symbol _ _gc_log in a asm file .
.globl _ _gc_log
when the aix as generated the xcoff object file, the visibility of the symbol will be unspecific , not default.
only the following asm has default visibility.
.globl _ _gc_log, exported

[DiggerLin]

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

[DiggerLin]

rebased the patch

[DiggerLin]

modify a test case

[jansvoboda11]

FYI, this might need rebasing on top of D97552.

[DiggerLin]

rebase code

[daltenty]

clang/lib/Frontend/CompilerInvocation.cpp

2733

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

[DiggerLin]

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

[DiggerLin]

clang/lib/Frontend/CompilerInvocation.cpp

2733

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.

[DiggerLin]

added extra comment back.

[sfertile]

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.

[lei]

This comment was removed by lei.

[DiggerLin]

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" ?

[jansvoboda11]

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.

[DiggerLin]

slight change , move to

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

to position as comment.

clang/lib/Frontend/CompilerInvocation.cpp

if (Invalid || GNUCVer.getBuild() || Minor >= 100 || Patch >= 100) {
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();
     }
     Opts.GNUCVersion = Major * 100 * 100 + Minor * 100 + Patch;
   }
 
   if (Args.hasArg(OPT_fgnu89_inline)) {
     if (Opts.CPlusPlus)
       Diags.Report(diag::err_drv_argument_not_allowed_with)
         << "-fgnu89-inline" << GetInputKindName(IK);
     else
       Opts.GNUInline = 1;
   }
 
   if (Args.hasArg(OPT_fapple_kext)) {
     if (!Opts.CPlusPlus)
       Diags.Report(diag::warn_c_kext);
#include "clang/Basic/LangStandards.def"
 
   // The type-visibility mode defaults to the value-visibility mode.
   if (Arg *typeVisOpt = Args.getLastArg(OPT_ftype_visibility)) {
     Opts.setTypeVisibilityMode(parseVisibility(typeVisOpt, Args, Diags));
   } else {
     Opts.setTypeVisibilityMode(Opts.getValueVisibilityMode());
   }
 
-  if (Args.hasArg(OPT_fvisibility_inlines_hidden))
+  if (Args.hasArg(OPT_fvisibility_inlines_hidden)) {
+    if (T.isOSAIX() && !Args.hasArg(OPT_fvisibility))
+      Opts.InlineVisibilityHidden = 0;
+    else
       Opts.InlineVisibilityHidden = 1;
+  }
 
   if (Args.hasArg(OPT_fvisibility_inlines_hidden_static_local_var))
     Opts.VisibilityInlinesHiddenStaticLocalVar = 1;
 
   if (Args.hasArg(OPT_fvisibility_global_new_delete_hidden))
     Opts.GlobalAllocationFunctionVisibilityHidden = 1;
 
   if (Args.hasArg(OPT_fapply_global_visibility_to_externs))

clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

+// REQUIRES: powerpc-registered-target
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -o - -x c++ -S  %s  |\
+// RUN:   FileCheck --check-prefixes=NOP-ASM,COMMON-ASM %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden -o - -x c++ -S %s  | \
+// RUN: FileCheck --check-prefixes=NOP-ASM,COMMON-ASM %s
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden  -fvisibility default -o - -x c++ -S %s  | \
+// RUN: FileCheck --check-prefix=COMMON-ASM %s
+
+// 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 -fvisibility default -emit-llvm -o - -x c++ %s  | \
+// RUN: FileCheck -check-prefixes=COMMON-IR,VISIBILITY-IR %s
+
+int x = 66;
+__attribute__((__noinline__)) inline void f() {
+  x = 55;
+}
+int bar() {
+  f();
+  return x;
+}
+
+// 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
+
+// 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
+
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -fvisibility-inlines-hidden  -fvisibility default -o - -x c++ -S %s  | \
+// RUN: FileCheck --check-prefix=CHECK %s
+/// CHECK-NOT:     nop