[PATCH] D87451: add new clang option -mno-xcoff-visibility
daltenty added inline comments.
Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
DiggerLin wrote:
> jasonliu wrote:
> > DiggerLin wrote:
> > > daltenty wrote:
> > > > This seems like it needs the corresponding comand-line option for llc
> > > > added and an llc test.
> > > I think it will be in another separate patch.
> > I would actually prefer to have that in the same patch, as that would give
> > us a full picture. It's not a huge patch even if we combine them.
> yes, it is not huge patch, one patch for the clang with option
> -mno-xcoff-visibility, another patch for llc option -no-xcoff-visibility , I
> think it is different functionality. and I have post the
> https://reviews.llvm.org/D88234 "add new option -no-xcoff-visibility for llc"
But the problem is this patch actually introduces the backend functionality
with no test in the LLVM component itself, because the test here is Clang only.
Either the patches should be merged so both components get tests in one patch
or both refactored so we have one llc/LLVM patch and one clang patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
DiggerLin marked an inline comment as done.
DiggerLin added inline comments.
Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
jasonliu wrote:
> DiggerLin wrote:
> > daltenty wrote:
> > > This seems like it needs the corresponding comand-line option for llc
> > > added and an llc test.
> > I think it will be in another separate patch.
> I would actually prefer to have that in the same patch, as that would give us
> a full picture. It's not a huge patch even if we combine them.
yes, it is not huge patch, one patch for the clang with option
-mno-xcoff-visibility, another patch for llc option -no-xcoff-visibility , I
think it is different functionality. and I have post the
https://reviews.llvm.org/D88234 "add new option -no-xcoff-visibility for llc"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
jasonliu added inline comments.
Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
DiggerLin wrote:
> daltenty wrote:
> > This seems like it needs the corresponding comand-line option for llc added
> > and an llc test.
> I think it will be in another separate patch.
I would actually prefer to have that in the same patch, as that would give us a
full picture. It's not a huge patch even if we combine them.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
jasonliu added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2310 +.. option:: -mno-xcoff-visibility + It's rare to see an option with only the negative form. Could we rename and make it a positive form somehow? Also we would need to move this option to where the m group belongs. Comment at: clang/test/CodeGen/aix-no-xcoff-visibility.cpp:1 +// RUN: %clang -target powerpc-unknown-aix -emit-llvm -o - -S %s |\ +// RUN: FileCheck --check-prefix=VISIBILITY-IR %s I don't think we should call the driver directly in here. We should have a separate driver test where we invoke `clang`, and we should invoke the front end `clang_cc1` here. Comment at: clang/test/CodeGen/aix-no-xcoff-visibility.cpp:75 + +// VISIBILITY-IR:@b = protected global i32 0 +// VISIBILITY-IR:@pramb = hidden global i32 0 Not sure if the IR check is really necessary, since we haven't made any IR change here. It's going to be all the same with or without the new -m option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87451/new/ https://reviews.llvm.org/D87451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
DiggerLin updated this revision to Diff 294070.
DiggerLin marked an inline comment as done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
Files:
clang/docs/ClangCommandLineReference.rst
clang/include/clang/Basic/CodeGenOptions.def
clang/include/clang/Driver/Options.td
clang/lib/CodeGen/BackendUtil.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/aix-no-xcoff-visibility.cpp
llvm/include/llvm/Target/TargetMachine.h
llvm/include/llvm/Target/TargetOptions.h
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
Index: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
===
--- llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -1686,17 +1686,19 @@
assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid.");
MCSymbolAttr VisibilityAttr = MCSA_Invalid;
- switch (GV->getVisibility()) {
+ if (!TM.getNoXCOFFVisibility()) {
+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;
+// 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;
+}
}
OutStreamer->emitXCOFFSymbolLinkageWithVisibility(GVSym, LinkageAttr,
Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -123,9 +123,10 @@
EnableFastISel(false), EnableGlobalISel(false), UseInitArray(false),
DisableIntegratedAS(false), RelaxELFRelocations(false),
FunctionSections(false), DataSections(false),
- UniqueSectionNames(true), UniqueBasicBlockSectionNames(false),
- TrapUnreachable(false), NoTrapAfterNoreturn(false), TLSSize(0),
- EmulatedTLS(false), ExplicitEmulatedTLS(false), EnableIPRA(false),
+ NoXCOFFVisibility(false), UniqueSectionNames(true),
+ UniqueBasicBlockSectionNames(false), TrapUnreachable(false),
+ NoTrapAfterNoreturn(false), TLSSize(0), EmulatedTLS(false),
+ ExplicitEmulatedTLS(false), EnableIPRA(false),
EmitStackSizeSection(false), EnableMachineOutliner(false),
EnableMachineFunctionSplitter(false), SupportsDefaultOutlining(false),
EmitAddrsig(false), EmitCallSiteInfo(false),
@@ -230,6 +231,9 @@
/// Emit data into separate sections.
unsigned DataSections : 1;
+/// Do not emit visibility attribute for xcoff.
+unsigned NoXCOFFVisibility : 1;
+
unsigned UniqueSectionNames : 1;
/// Use unique names for basic block sections.
Index: llvm/include/llvm/Target/TargetMachine.h
===
--- llvm/include/llvm/Target/TargetMachine.h
+++ llvm/include/llvm/Target/TargetMachine.h
@@ -260,6 +260,10 @@
return Options.FunctionSections;
}
+ /// Return true if visibility attribute should not be emitted in xcoff,
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
/// If basic blocks should be emitted into their own section,
/// corresponding to -fbasic-block-sections.
llvm::BasicBlockSection getBBSectionsType() const {
Index: clang/test/CodeGen/aix-no-xcoff-visibility.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-no-xcoff-visibility.cpp
@@ -0,0 +1,110 @@
+// RUN: %clang -target powerpc-unknown-aix -emit-llvm -o - -S %s |\
+// RUN: FileCheck --check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang -target powerpc-unknown-aix -o - -S %s |\
+// RUN: FileCheck --check-prefix=NOVISIBILITY-ASM %s
+
+// RUN: %clang -target powerpc-unknown-linux -emit-llvm -o - -S %s | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang -mno-xcoff-visibility -target powerpc-unknown-aix -emit-llvm -o - -S %s | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang -mno-xcoff-visibility -target powerpc-unknown-aix -o - -S %s | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-ASM %s
+
+// RUN: not %clang -mno-xcoff-visibility -target powerpc-unknown-linux -emit-llvm -o - -S %s 2>&1 | \
+// RUN: FileCheck -check-prefix=ERROR %s
+
+// RUN: %clang
[PATCH] D87451: add new clang option -mno-xcoff-visibility
DiggerLin marked 5 inline comments as done.
DiggerLin added inline comments.
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2768
// The type-visibility mode defaults to the value-visibility mode.
if (Arg *typeVisOpt = Args.getLastArg(OPT_ftype_visibility)) {
Opts.setTypeVisibilityMode(parseVisibility(typeVisOpt, Args, Diags));
jasonliu wrote:
> Question: how should -mignore-xcoff-visiblity interact with -ftype-visibility?
the -ftype-visibility do not support in the clang, so there is no interact.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
DiggerLin marked an inline comment as done.
DiggerLin added inline comments.
Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
daltenty wrote:
> This seems like it needs the corresponding comand-line option for llc added
> and an llc test.
I think it will be in another separate patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
daltenty added inline comments.
Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
This seems like it needs the corresponding comand-line option for llc added and
an llc test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87451: add new clang option -mno-xcoff-visibility
DiggerLin updated this revision to Diff 293571.
DiggerLin added a comment.
Herald added subscribers: llvm-commits, kbarton, hiraditya, nemanjai.
Herald added a project: LLVM.
address comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87451/new/
https://reviews.llvm.org/D87451
Files:
clang/docs/ClangCommandLineReference.rst
clang/include/clang/Basic/CodeGenOptions.def
clang/include/clang/Driver/Options.td
clang/lib/CodeGen/BackendUtil.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/Driver/ignore-xcoff-visibility.cpp
llvm/include/llvm/Target/TargetMachine.h
llvm/include/llvm/Target/TargetOptions.h
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
Index: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
===
--- llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -1686,17 +1686,19 @@
assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid.");
MCSymbolAttr VisibilityAttr = MCSA_Invalid;
- switch (GV->getVisibility()) {
+ if (!TM.getNoXCOFFVisibility()) {
+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;
+// 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;
+}
}
OutStreamer->emitXCOFFSymbolLinkageWithVisibility(GVSym, LinkageAttr,
Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -123,9 +123,10 @@
EnableFastISel(false), EnableGlobalISel(false), UseInitArray(false),
DisableIntegratedAS(false), RelaxELFRelocations(false),
FunctionSections(false), DataSections(false),
- UniqueSectionNames(true), UniqueBasicBlockSectionNames(false),
- TrapUnreachable(false), NoTrapAfterNoreturn(false), TLSSize(0),
- EmulatedTLS(false), ExplicitEmulatedTLS(false), EnableIPRA(false),
+ NoXCOFFVisibility(false), UniqueSectionNames(true),
+ UniqueBasicBlockSectionNames(false), TrapUnreachable(false),
+ NoTrapAfterNoreturn(false), TLSSize(0), EmulatedTLS(false),
+ ExplicitEmulatedTLS(false), EnableIPRA(false),
EmitStackSizeSection(false), EnableMachineOutliner(false),
EnableMachineFunctionSplitter(false), SupportsDefaultOutlining(false),
EmitAddrsig(false), EmitCallSiteInfo(false),
@@ -230,6 +231,9 @@
/// Emit data into separate sections.
unsigned DataSections : 1;
+/// Do not emit visibility attribute for xcoff.
+unsigned NoXCOFFVisibility : 1;
+
unsigned UniqueSectionNames : 1;
/// Use unique names for basic block sections.
Index: llvm/include/llvm/Target/TargetMachine.h
===
--- llvm/include/llvm/Target/TargetMachine.h
+++ llvm/include/llvm/Target/TargetMachine.h
@@ -260,6 +260,10 @@
return Options.FunctionSections;
}
+ /// Return true if visibility attribute should not be emitted in xcoff,
+ /// corresponding to -mno-xcoff-visibility.
+ bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
/// If basic blocks should be emitted into their own section,
/// corresponding to -fbasic-block-sections.
llvm::BasicBlockSection getBBSectionsType() const {
Index: clang/test/Driver/ignore-xcoff-visibility.cpp
===
--- /dev/null
+++ clang/test/Driver/ignore-xcoff-visibility.cpp
@@ -0,0 +1,110 @@
+// RUN: %clang -target powerpc-unknown-aix -emit-llvm -o - -S %s |\
+// RUN: FileCheck --check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang -target powerpc-unknown-aix -o - -S %s |\
+// RUN: FileCheck --check-prefix=NOVISIBILITY-ASM %s
+
+// RUN: %clang -target powerpc-unknown-linux -emit-llvm -o - -S %s | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang -mno-xcoff-visibility -target powerpc-unknown-aix -emit-llvm -o - -S %s | \
+// RUN: FileCheck -check-prefix=VISIBILITY-IR %s
+
+// RUN: %clang -mno-xcoff-visibility -target powerpc-unknown-aix -o - -S %s | \
+// RUN: FileCheck -check-prefix=NOVISIBILITY-ASM %s
+
+// RUN: not %clang -mno-xcoff-visibility -target powerpc-unknown-linux
