Page MenuHomePhabricator
Differential D89986 Diff 301029 clang/test/CodeGen/aix-visibility-inlines-hidden.cpp

Changeset View

Standalone View

View Options

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

  • This file was added.
// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large -emit-llvm -o - -x c++ %s | \
// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
sfertileUnsubmitted
Done
ReplyInline Actions

Shouldn't need this requires either.

sfertile: Shouldn't need this requires either.
jasonliuUnsubmitted
Done
ReplyInline Actions

Do you need this line?

jasonliu: Do you need this line?
DiggerLinAuthorUnsubmitted
Done
ReplyInline Actions

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: there is a comment in https://reviews.llvm.org/D87451 " Hello. I added a power-pc REQUIRES…
sfertileUnsubmitted
Done
ReplyInline Actions

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

sfertile: Look at the run steps in the test when that was added. eg: ``` // RUN: %clang_cc1 -triple…
// RUN: %clang_cc1 -triple powerpc-unknown-aix -mcmodel=large \
// RUN: -fvisibility-inlines-hidden -emit-llvm -o - -x c++ %s | \
sfertileUnsubmitted
Done
ReplyInline Actions

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

sfertile: Can we drop the COMMON-IR part of the test? I don't think it adds much value but clutters the…
// RUN: FileCheck -check-prefix=NOVISIBILITY-IR %s
sfertileUnsubmitted
Done
ReplyInline Actions

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

sfertile: Can you break up this RUN step similar to how you formatted the following one.
// 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
sfertileUnsubmitted
Done
ReplyInline Actions

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

sfertile: There is trailing white space at the end of this line.
int x = 66;
__attribute__((__noinline__)) inline void f() {
x = 55;
}
#pragma GCC visibility push(hidden)
hubert.reinterpretcastUnsubmitted
Done
ReplyInline Actions

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

hubert.reinterpretcast: There's no testing for the interaction with `-mignore-xcoff-visibility`.
__attribute__((__noinline__)) inline void foo() {
x = 55;
}
#pragma GCC visibility pop
int bar() {
f();
foo();
return x;
hubert.reinterpretcastUnsubmitted
Done
ReplyInline Actions

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;
}
hubert.reinterpretcast: The general issue is that the `nop` instruction is being omitted even when the visibility…
}
// VISIBILITY-IR: define linkonce_odr hidden void @_Z1fv()
// NOVISIBILITY-IR: define linkonce_odr void @_Z1fv()
rsmithUnsubmitted
Done
ReplyInline Actions

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.

rsmith: Generally we strongly prefer for frontend tests to test only the frontend; in this case, that…
nemanjaiUnsubmitted
Done
ReplyInline Actions

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

nemanjai: +1 The asm test can go into `llvm/test/CodeGen/PowerPC`.
DiggerLinAuthorUnsubmitted
Done
ReplyInline Actions

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: If I put the ASM test as separate test case in the llvm/test/CodeGen/PowerPC .it will have…
lebedev.riUnsubmitted
Done
ReplyInline Actions

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

lebedev.ri: That's precisely the point. Clang codegen tests should only test clang IR generation. They…
// VISIBILITY-IR: define linkonce_odr hidden void @_Z3foov()
// NOVISIBILITY-IR: define linkonce_odr void @_Z3foov()