Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

DiggerLin created this revision.Oct 22 2020, 2:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 2:26 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
DiggerLin requested review of this revision.Oct 22 2020, 2:26 PM
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 added a subscriber: rsmith.Oct 22 2020, 5:30 PM
rsmith added inline comments.
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 added inline comments.
clang/test/CodeGen/aix-visibility-inlines-hidden.cpp
31–35

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

DiggerLin added inline comments.Oct 23 2020, 12:46 PM
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

address comment

DiggerLin retitled this revision from [AIX]ignore the option -fvisibility-inlines-hidden when there is no option -fvisibility=* to [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility.Oct 23 2020, 12:52 PM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin marked 4 inline comments as done.
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
520

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
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 added inline comments.Oct 26 2020, 10:37 AM
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 marked 2 inline comments as done.Oct 26 2020, 10:44 AM
DiggerLin added inline comments.
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 added inline comments.Oct 26 2020, 11:35 AM
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 marked 2 inline comments as done.Oct 26 2020, 11:45 AM
DiggerLin added inline comments.
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 marked an inline comment as done.Oct 26 2020, 11:48 AM
DiggerLin added inline comments.
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 added inline comments.Oct 26 2020, 12:05 PM
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 added inline comments.Oct 26 2020, 12:06 PM
clang/lib/CodeGen/BackendUtil.cpp
520

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

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

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
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
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
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 marked an inline comment as done.Oct 27 2020, 8:56 AM
DiggerLin added inline comments.
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 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
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).