Page MenuHomePhabricator

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

Authored by DiggerLin on Oct 22 2020, 2:26 PM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin marked 4 inline comments as done.Oct 23 2020, 12:52 PM
lebedev.ri added inline comments.
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 updated this revision to Diff 300421.Oct 23 2020, 3:14 PM

delete asm test case from clang/test

DiggerLin marked an inline comment as done.Oct 23 2020, 3:15 PM
jasonliu added inline comments.Oct 26 2020, 7:44 AM
clang/lib/CodeGen/BackendUtil.cpp
552

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

DiggerLin marked an inline comment as done.Oct 26 2020, 7:57 AM
DiggerLin added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
552

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 added inline comments.Oct 26 2020, 10:37 AM
clang/lib/CodeGen/BackendUtil.cpp
552

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 marked 2 inline comments as done.Oct 26 2020, 10:44 AM
DiggerLin added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
552

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 added inline comments.Oct 26 2020, 11:35 AM
clang/lib/CodeGen/BackendUtil.cpp
552

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 marked 2 inline comments as done.Oct 26 2020, 11:45 AM
DiggerLin added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
552

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 marked an inline comment as done.Oct 26 2020, 11:48 AM
DiggerLin added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
552

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 added inline comments.Oct 26 2020, 12:05 PM
clang/lib/CodeGen/BackendUtil.cpp
552

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 added inline comments.Oct 26 2020, 12:06 PM
clang/lib/CodeGen/BackendUtil.cpp
552

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

jasonliu accepted this revision.Oct 26 2020, 1:34 PM

LGTM with minor nit.

clang/lib/AST/Decl.cpp
1490–1496

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?

This revision is now accepted and ready to land.Oct 26 2020, 1:34 PM
sfertile added inline comments.Oct 26 2020, 1:57 PM
clang/lib/CodeGen/BackendUtil.cpp
552

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–2

You shouldn't need this requires anymore.

74

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.

Sorry, missed a comment.

sfertile added inline comments.Oct 26 2020, 2:03 PM
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 marked 9 inline comments as done.Oct 27 2020, 6:12 AM
DiggerLin added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
552

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–2
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 updated this revision to Diff 300972.Oct 27 2020, 6:16 AM
DiggerLin marked 3 inline comments as done.

address comment

sfertile added inline comments.Oct 27 2020, 8:49 AM
clang/lib/CodeGen/BackendUtil.cpp
552

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 marked an inline comment as done.Oct 27 2020, 8:56 AM
DiggerLin added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
552

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 updated this revision to Diff 301020.Oct 27 2020, 8:57 AM
DiggerLin marked an inline comment as done.
DiggerLin updated this revision to Diff 301029.Oct 27 2020, 9:17 AM
DiggerLin marked an inline comment as done.

remove REQUIRES: powerpc-registered-target from test case

clang/lib/CodeGen/BackendUtil.cpp
552

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 added inline comments.Jan 18 2021, 6:47 AM
clang/lib/CodeGen/BackendUtil.cpp
552
  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"
  1. 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

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

DiggerLin updated this revision to Diff 327804.Mar 3 2021, 8:18 AM

rebased the patch

DiggerLin updated this revision to Diff 327898.Mar 3 2021, 1:10 PM

modify a test case

FYI, this might need rebasing on top of D97552.

DiggerLin updated this revision to Diff 328632.Mar 5 2021, 1:24 PM

rebase code

daltenty added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
3633

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

DiggerLin updated this revision to Diff 328769.Mar 6 2021, 8:59 AM

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

DiggerLin marked an inline comment as done.Mar 8 2021, 6:39 AM
clang/lib/Frontend/CompilerInvocation.cpp
3633

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 updated this revision to Diff 328998.Mar 8 2021, 6:42 AM
DiggerLin marked an inline comment as done.

added extra comment back.

sfertile accepted this revision.Mar 8 2021, 9:47 AM
sfertile added a subscriber: Xiangling_L.

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 added a subscriber: lei.Mar 8 2021, 11:40 AM
This comment was removed by lei.

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

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 updated this revision to Diff 329316.Mar 9 2021, 6:40 AM

slight change , move to

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

to position as comment.

This revision was landed with ongoing or failed builds.Mar 9 2021, 7:38 AM
This revision was automatically updated to reflect the committed changes.