[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

21

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

30

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

31–35

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

31–35

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

[DiggerLin]

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

31–35

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

31–35

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

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

[DiggerLin]

clang/lib/CodeGen/BackendUtil.cpp

520

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

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

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

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

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

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

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

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

[jasonliu]

LGTM with minor nit.

clang/lib/AST/Decl.cpp

1481–1487

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

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

0–1

You shouldn't need this requires anymore.

65

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

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

0–1

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

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

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

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

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