[PATCH] D87451: add new option ignore-xcoff-visibility
DiggerLin updated this revision to Diff 296267. DiggerLin marked 3 inline comments as done. DiggerLin added a comment. 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/CodeGen/aix-ignore-xcoff-visibility.cpp clang/test/Driver/ignore-xcoff-visibility.cpp llvm/include/llvm/CodeGen/CommandFlags.h llvm/include/llvm/Target/TargetMachine.h llvm/include/llvm/Target/TargetOptions.h llvm/lib/CodeGen/CommandFlags.cpp llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll Index: llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll === --- /dev/null +++ llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll @@ -0,0 +1,48 @@ +; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec < %s | \ +; RUN: FileCheck --check-prefix=VISIBILITY-ASM %s +; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \ +; RUN: FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s +; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec < %s | \ +; RUN: FileCheck --check-prefix=VISIBILITY-ASM %s +; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \ +; RUN: FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s + +@foo_p = global void ()* @zoo_extern_h, align 4 +@b = protected global i32 0, align 4 + +define hidden void @foo_h(i32* %p) { +entry: + %p.addr = alloca i32*, align 4 + store i32* %p, i32** %p.addr, align 4 + %0 = load i32*, i32** %p.addr, align 4 + %1 = load i32, i32* %0, align 4 + %inc = add nsw i32 %1, 1 + store i32 %inc, i32* %0, align 4 + ret void +} + +declare hidden void @zoo_extern_h() + +define protected void @bar() { +entry: + call void @foo_h(i32* @b) + %0 = load void ()*, void ()** @foo_p, align 4 + call void %0() + ret void +} + +; VISIBILITY-ASM: .globl foo_h[DS],hidden +; VISIBILITY-ASM: .globl .foo_h,hidden +; VISIBILITY-ASM: .globl bar[DS],protected +; VISIBILITY-ASM: .globl .bar,protected +; VISIBILITY-ASM: .globl b,protected + +; IGNOREVISIBILITY-ASM: .globl foo_h[DS] +; IGNOREVISIBILITY-ASM: .globl .foo_h +; IGNOREVISIBILITY-ASM: .globl bar[DS] +; IGNOREVISIBILITY-ASM: .globl .bar +; IGNOREVISIBILITY-ASM: .globl b Index: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp === --- llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp +++ llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp @@ -1702,17 +1702,19 @@ assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid."); MCSymbolAttr VisibilityAttr = MCSA_Invalid; - switch (GV->getVisibility()) { + 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; +// 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/lib/CodeGen/CommandFlags.cpp === --- llvm/lib/CodeGen/CommandFlags.cpp +++ llvm/lib/CodeGen/CommandFlags.cpp @@ -74,6 +74,7 @@ CGOPT(bool, RelaxELFRelocations) CGOPT_EXP(bool, DataSections) CGOPT_EXP(bool, FunctionSections) +CGOPT(bool, IgnoreXCOFFVisibility) CGOPT(std::string, BBSections) CGOPT(unsigned, TLSSize) CGOPT(bool, EmulatedTLS) @@ -333,6 +334,13 @@ cl::init(false)); CGBINDOPT(FunctionSections); + static cl::opt IgnoreXCOFFVisibility( + "ignore-xcoff-visibility", + cl::desc("Not emit the visibility attribute for asm in AIX OS or give " + "all symbols 'unspecified' visibility in xcoff object file"), + cl::init(false)); + CGBINDOPT(IgnoreXCOFFVisibility); + static cl::opt BBSections( "basic-block-sections", cl::desc("Emit basic blocks into separate
[PATCH] D87451: add new option ignore-xcoff-visibility
DiggerLin marked 7 inline comments as done. DiggerLin added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2622 + +Do not emit any visibility attribute for asm on AIX or give all symbols 'unspecified' visibility in xcoff object file(XCOFF only) + daltenty wrote: > daltenty wrote: > > nit: add a space before parens > I don't think the object file writing case was handled yet? This makes it > sound like it is. yes, we do not implement the visibility of the object file in the XCoffObjectWriter.cpp Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5242 + if (const Arg *A = Args.getLastArg(options::OPT_mignore_xcoff_visibility)) { +if (Triple.isOSAIX()) daltenty wrote: > Use `Args.hasFlag` instead, since this option doesn't have a value we need to > check. we need to get the value here D.Diag(diag::err_drv_unsupported_opt_for_target) << A->getAsString(Args) << TripleStr; Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:72 + +// ERROR: unsupported option '-mignore-xcoff-visibility' for target 'powerpc-unknown-linux' + daltenty wrote: > This isn't being checked anymore, also probably belongs in the other file thanks 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 option ignore-xcoff-visibility
daltenty added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2622 + +Do not emit any visibility attribute for asm on AIX or give all symbols 'unspecified' visibility in xcoff object file(XCOFF only) + nit: add a space before parens Comment at: clang/docs/ClangCommandLineReference.rst:2622 + +Do not emit any visibility attribute for asm on AIX or give all symbols 'unspecified' visibility in xcoff object file(XCOFF only) + daltenty wrote: > nit: add a space before parens I don't think the object file writing case was handled yet? This makes it sound like it is. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5242 + if (const Arg *A = Args.getLastArg(options::OPT_mignore_xcoff_visibility)) { +if (Triple.isOSAIX()) Use `Args.hasFlag` instead, since this option doesn't have a value we need to check. Comment at: clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp:72 + +// ERROR: unsupported option '-mignore-xcoff-visibility' for target 'powerpc-unknown-linux' + This isn't being checked anymore, also probably belongs in the other file Comment at: clang/test/Driver/ignore-xcoff-visibility.cpp:2 +// RUN: %clang -### -target powerpc-unknown-aix -mignore-xcoff-visibility -S %s 2> %t.log +// RUN: FileCheck -check-prefix=CHECK %s < %t.log +// CHECK: "-mignore-xcoff-visibility" We should check the diagnostic here Comment at: clang/test/Driver/ignore-xcoff-visibility.cpp:3 +// RUN: FileCheck -check-prefix=CHECK %s < %t.log +// CHECK: "-mignore-xcoff-visibility" nit: We should constrain this to be following the cc1 invocation 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 option ignore-xcoff-visibility
DiggerLin updated this revision to Diff 295838. DiggerLin marked an inline comment as done. DiggerLin added a comment. address comment and merger the patch https://reviews.llvm.org/D88234 into the patch 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-ignore-xcoff-visibility.cpp clang/test/Driver/ignore-xcoff-visibility.cpp llvm/include/llvm/CodeGen/CommandFlags.h llvm/include/llvm/Target/TargetMachine.h llvm/include/llvm/Target/TargetOptions.h llvm/lib/CodeGen/CommandFlags.cpp llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll Index: llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll === --- /dev/null +++ llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll @@ -0,0 +1,48 @@ +; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec < %s | \ +; RUN: FileCheck --check-prefix=VISIBILITY-ASM %s +; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \ +; RUN: FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s +; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec < %s | \ +; RUN: FileCheck --check-prefix=VISIBILITY-ASM %s +; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \ +; RUN: -mattr=-altivec -ignore-xcoff-visibility < %s | \ +; RUN: FileCheck --check-prefix=IGNOREVISIBILITY-ASM %s + +@foo_p = global void ()* @zoo_extern_h, align 4 +@b = protected global i32 0, align 4 + +define hidden void @foo_h(i32* %p) { +entry: + %p.addr = alloca i32*, align 4 + store i32* %p, i32** %p.addr, align 4 + %0 = load i32*, i32** %p.addr, align 4 + %1 = load i32, i32* %0, align 4 + %inc = add nsw i32 %1, 1 + store i32 %inc, i32* %0, align 4 + ret void +} + +declare hidden void @zoo_extern_h() + +define protected void @bar() { +entry: + call void @foo_h(i32* @b) + %0 = load void ()*, void ()** @foo_p, align 4 + call void %0() + ret void +} + +; VISIBILITY-ASM: .globl foo_h[DS],hidden +; VISIBILITY-ASM: .globl .foo_h,hidden +; VISIBILITY-ASM: .globl bar[DS],protected +; VISIBILITY-ASM: .globl .bar,protected +; VISIBILITY-ASM: .globl b,protected + +; IGNOREVISIBILITY-ASM: .globl foo_h[DS] +; IGNOREVISIBILITY-ASM: .globl .foo_h +; IGNOREVISIBILITY-ASM: .globl bar[DS] +; IGNOREVISIBILITY-ASM: .globl .bar +; IGNOREVISIBILITY-ASM: .globl b Index: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp === --- llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp +++ llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp @@ -1702,17 +1702,19 @@ assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid."); MCSymbolAttr VisibilityAttr = MCSA_Invalid; - switch (GV->getVisibility()) { + 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; +// 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/lib/CodeGen/CommandFlags.cpp === --- llvm/lib/CodeGen/CommandFlags.cpp +++ llvm/lib/CodeGen/CommandFlags.cpp @@ -74,6 +74,7 @@ CGOPT(bool, RelaxELFRelocations) CGOPT_EXP(bool, DataSections) CGOPT_EXP(bool, FunctionSections) +CGOPT(bool, IgnoreXCOFFVisibility) CGOPT(std::string, BBSections) CGOPT(unsigned, TLSSize) CGOPT(bool, EmulatedTLS) @@ -333,6 +334,13 @@ cl::init(false)); CGBINDOPT(FunctionSections); + static cl::opt IgnoreXCOFFVisibility( + "ignore-xcoff-visibility", + cl::desc("Not emit the visibility attribute for asm in AIX OS or give " + "all symbols 'unspecified' visibility in xcoff object file"), + cl::init(false)); + CGBINDOPT(IgnoreXCOFFVisibility); + static cl::opt BBSections(
[PATCH] D87451: add new option ignore-xcoff-visibility
DiggerLin marked 5 inline comments as done. DiggerLin added inline comments. 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 jasonliu wrote: > 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. IR output will not change in the new option. I think we need to check whether the IR be changed in the new 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