[AIX][TOC] Add -mtocdata/-mno-tocdata options on AIX #67999
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
syzaara
requested review from
MaskRay,
hubert-reinterpretcast,
mandlebug and
stephenpeckham
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-powerpc @llvm/pr-subscribers-mc ChangesThis patch enables support that the XL compiler had for AIX under -qdatalocal/-qdataimported. Patch is 38.39 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67999.diff 22 Files Affected:
diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index 25bbd72c81f7a3b..ee37872771c352a 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -4016,7 +4016,63 @@ Clang expects the GCC executable "gcc.exe" compiled for
AIX
^^^
+TOC Data Transformation
+"""""""""""""""""""""""
+TOC data transformation is off by default (``-mno-tocdata``).
+When ``-mtocdata`` is specified, the TOC data transformation will be applied to
+all suitable variables with static storage duration, including static data
+members of classes and block-scope static variables (if not marked as exceptions,
+see further below).
+Suitable variables must:
+
+- have complete types
+- be independently generated (i.e., not placed in a pool)
+- be at most as large as a pointer
+- not be aligned more strictly than a pointer
+- not be structs containing flexible array members
+- not have internal linkage
+- not have aliases
+- not have section attributes
+
+The TOC data transformation results in the variable, not its address,
+being placed in the TOC. This eliminates the need to load the address of the
+variable from the TOC.
+
+Note:
+If the TOC data transformation is applied to a variable whose definition
+is imported, the linker will generate fixup code for reading or writing to the
+variable.
+
+Any internal linkage variables specified to any TOC data options will be ignored.
+
+**Options:**
+
+.. option:: -mno-tocdata
+
+ This is the default behaviour. Only variables explicitly specified with
+ ``-mtocdata=`` will have the TOC data transformation applied.
+
+.. option:: -mtocdata
+
+ Apply the TOC data transformation to all suitable variables with static
+ storage duration (including static data members of classes and block-scope
+ static variables) that are not explicitly specified with ``-mno-tocdata=``.
+
+.. option:: -mno-tocdata=
+
+ Can be used in conjunction with ``-mtocdata`` to mark the comma-separated
+ list of external linkage variables, specified using their mangled names, as
+ exceptions to ``-mtocdata``.
+
+.. option:: -mtocdata=
+
+ Apply the TOC data transformation to the comma-separated list of external
+ linkage variables, specified using their mangled names, if they are suitable.
+ Emit diagnostics for all unsuitable variables specified.
+
+Default Visibility Export Mapping
+"""""""""""""""""""""""""""""""""
The ``-mdefault-visibility-export-mapping=`` option can be used to control
mapping of default visibility to an explicit shared object export
(i.e. XCOFF exported visibility). Three values are provided for the option:
diff --git a/clang/include/clang/Basic/CodeGenOptions.h b/clang/include/clang/Basic/CodeGenOptions.h
index 12be4e0025a7054..3af1a5d5aeec379 100644
--- a/clang/include/clang/Basic/CodeGenOptions.h
+++ b/clang/include/clang/Basic/CodeGenOptions.h
@@ -408,6 +408,15 @@ class CodeGenOptions : public CodeGenOptionsBase {
/// List of dynamic shared object files to be loaded as pass plugins.
std::vector<std::string> PassPlugins;
+ /// List of global variables explicitly specified by the user as toc-data.
+ std::vector<std::string> TocDataVarsUserSpecified;
+
+ /// List of global variables that over-ride the toc-data default.
+ std::vector<std::string> NoTocDataVars;
+
+ /// Flag for all global variables to be treated as toc-data.
+ bool AllTocData;
+
/// Path to allowlist file specifying which objects
/// (files, functions) should exclusively be instrumented
/// by sanitizer coverage pass.
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 2a48c063e243ee0..736fb89b2ff5e8f 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -560,6 +560,9 @@ def warn_drv_unsupported_gpopt : Warning<
"ignoring '-mgpopt' option as it cannot be used with %select{|the implicit"
" usage of }0-mabicalls">,
InGroup<UnsupportedGPOpt>;
+def warn_drv_unsupported_tocdata: Warning<
+ "ignoring '-mtocdata' as it is only supported for -mcmodel=small">,
+ InGroup<OptionIgnored>;
def warn_drv_unsupported_sdata : Warning<
"ignoring '-msmall-data-limit=' with -mcmodel=large for -fpic or RV64">,
InGroup<OptionIgnored>;
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 715e0c0dc8fa84e..7269ad307eb9551 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -93,6 +93,8 @@ def err_fe_backend_error_attr :
def warn_fe_backend_warning_attr :
Warning<"call to '%0' declared with 'warning' attribute: %1">, BackendInfo,
InGroup<BackendWarningAttributes>;
+def warn_toc_unsupported_type : Warning<"The -mtocdata option is ignored "
+ "for %0 because %1.">, InGroup<BackendWarningAttributes>;
def err_fe_invalid_code_complete_file : Error<
"cannot locate code-completion file %0">, DefaultFatal;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index ee4e23f335e7875..4c1bc268259f1c7 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3426,6 +3426,29 @@ def fpass_plugin_EQ : Joined<["-"], "fpass-plugin=">,
MetaVarName<"<dsopath>">,
HelpText<"Load pass plugin from a dynamic shared object file (only with new pass manager).">,
MarshallingInfoStringVector<CodeGenOpts<"PassPlugins">>;
+def mtocdata : Flag<["-"], "mtocdata">,
+ Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
+ Flags<[TargetSpecific]>,
+ HelpText<"All suitable variables will have the TOC data transformation applied.">,
+ MarshallingInfoFlag<CodeGenOpts<"AllTocData">>;
+def mno_tocdata : Flag<["-"], "mno-tocdata">,
+ Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
+ Flags<[TargetSpecific]>,
+ HelpText<"This is the default. TOC data transformation is not applied to any"
+ "variables. Only variables specified explicitly in -mtocdata= will"
+ "have the TOC data transformation.">;
+def mtocdata_EQ : CommaJoined<["-"], "mtocdata=">,
+ Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
+ Flags<[TargetSpecific]>,
+ HelpText<"Specifies a list of variables to which the TOC data transformation"
+ "will be applied.">,
+ MarshallingInfoStringVector<CodeGenOpts<"TocDataVarsUserSpecified">>;
+def mno_tocdata_EQ : CommaJoined<["-"], "mno-tocdata=">,
+ Visibility<[ClangOption, CLOption, DXCOption, CC1Option]>,
+ Flags<[TargetSpecific]>,
+ HelpText<"Specifies a list of variables to be exempt from the TOC data"
+ "transformation.">,
+ MarshallingInfoStringVector<CodeGenOpts<"NoTocDataVars">>;
defm preserve_as_comments : BoolFOption<"preserve-as-comments",
CodeGenOpts<"PreserveAsmComments">, DefaultTrue,
NegFlag<SetFalse, [], [ClangOption, CC1Option],
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 99b94588f56f0a5..6805b514adb6d3f 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -285,6 +285,7 @@ llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl(
setTLSMode(GV, D);
setGVProperties(GV, &D);
+ getTargetCodeGenInfo().setTargetAttributes(cast<Decl>(&D), GV, *this);
// Make sure the result is of the correct type.
LangAS ExpectedAS = Ty.getAddressSpace();
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 93c3f8d6efbaade..eb00aba8a3c0756 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -622,6 +622,28 @@ static bool checkAliasedGlobal(
return true;
}
+// Emit a warning if toc-data attribute is requested for global variables that
+// have aliases and remove the toc-data attribute.
+static void checkAliasForTocData(llvm::GlobalVariable *GVar,
+ const CodeGenOptions &CodeGenOpts,
+ DiagnosticsEngine &Diags,
+ SourceLocation Location) {
+ if (GVar->hasAttribute("toc-data")) {
+ auto GVId = GVar->getGlobalIdentifier();
+ // Is this a global variable specified by the user as local?
+ bool UserSpecifiedTOC =
+ llvm::binary_search(CodeGenOpts.TocDataVarsUserSpecified, GVId);
+ if (UserSpecifiedTOC) {
+ Diags.Report(Location, diag::warn_toc_unsupported_type)
+ << GVId << "the variable has an alias";
+ }
+ llvm::AttributeSet CurrAttributes = GVar->getAttributes();
+ llvm::AttributeSet NewAttributes =
+ CurrAttributes.removeAttribute(GVar->getContext(), "toc-data");
+ GVar->setAttributes(NewAttributes);
+ }
+}
+
void CodeGenModule::checkAliases() {
// Check if the constructed aliases are well formed. It is really unfortunate
// that we have to do this in CodeGen, but we only construct mangled names
@@ -648,6 +670,12 @@ void CodeGenModule::checkAliases() {
continue;
}
+ if (getContext().getTargetInfo().getTriple().isOSAIX())
+ if (const llvm::GlobalVariable *GVar =
+ dyn_cast<const llvm::GlobalVariable>(GV))
+ checkAliasForTocData(const_cast<llvm::GlobalVariable *>(GVar),
+ getCodeGenOpts(), Diags, Location);
+
llvm::Constant *Aliasee =
IsIFunc ? cast<llvm::GlobalIFunc>(Alias)->getResolver()
: cast<llvm::GlobalAlias>(Alias)->getAliasee();
diff --git a/clang/lib/CodeGen/Targets/PPC.cpp b/clang/lib/CodeGen/Targets/PPC.cpp
index 40dddde508c1772..3032a9c1ee3c09f 100644
--- a/clang/lib/CodeGen/Targets/PPC.cpp
+++ b/clang/lib/CodeGen/Targets/PPC.cpp
@@ -8,6 +8,7 @@
#include "ABIInfoImpl.h"
#include "TargetInfo.h"
+#include "clang/Basic/DiagnosticFrontend.h"
using namespace clang;
using namespace clang::CodeGen;
@@ -145,6 +146,9 @@ class AIXTargetCodeGenInfo : public TargetCodeGenInfo {
bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
llvm::Value *Address) const override;
+
+ void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
+ CodeGen::CodeGenModule &M) const override;
};
} // namespace
@@ -265,6 +269,63 @@ bool AIXTargetCodeGenInfo::initDwarfEHRegSizeTable(
return PPC_initDwarfEHRegSizeTable(CGF, Address, Is64Bit, /*IsAIX*/ true);
}
+void AIXTargetCodeGenInfo::setTargetAttributes(
+ const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &M) const {
+ if (auto *GVar = dyn_cast<llvm::GlobalVariable>(GV)) {
+ auto GVId = M.getMangledName(dyn_cast<NamedDecl>(D));
+
+ // Is this a global variable specified by the user as toc-data?
+ bool UserSpecifiedTOC =
+ llvm::binary_search(M.getCodeGenOpts().TocDataVarsUserSpecified, GVId);
+ // Assumes the same variable cannot be in both TocVarsUserSpecified and
+ // NoTocVars.
+ if (UserSpecifiedTOC ||
+ ((M.getCodeGenOpts().AllTocData) &&
+ !llvm::binary_search(M.getCodeGenOpts().NoTocDataVars, GVId))) {
+ const unsigned long PointerSize =
+ GV->getParent()->getDataLayout().getPointerSizeInBits() / 8;
+ ASTContext &Context = D->getASTContext();
+ auto *VarD = dyn_cast<VarDecl>(D);
+ assert(VarD && "Invalid declaration of global variable.");
+
+ unsigned Alignment = Context.toBits(Context.getDeclAlign(D)) / 8;
+ const auto *Ty = VarD->getType().getTypePtr();
+ const RecordDecl *RDecl =
+ Ty->isRecordType() ? Ty->getAs<RecordType>()->getDecl() : nullptr;
+
+ bool EmitDiagnostic = UserSpecifiedTOC && GV->hasExternalLinkage();
+ if (!Ty || Ty->isIncompleteType()) {
+ if (EmitDiagnostic)
+ M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+ << GVId << "of incomplete type";
+ } else if (RDecl && RDecl->hasFlexibleArrayMember()) {
+ if (UserSpecifiedTOC)
+ M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+ << GVId << "it contains a flexible array member";
+ } else if (VarD->getTLSKind() != VarDecl::TLS_None) {
+ if (EmitDiagnostic)
+ M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+ << GVId << "of thread local storage model";
+ } else if (PointerSize < Context.getTypeInfo(VarD->getType()).Width / 8) {
+ if (EmitDiagnostic)
+ M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+ << GVId << "variable is larger than a pointer";
+ } else if (PointerSize < Alignment) {
+ if (EmitDiagnostic)
+ M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+ << GVId << "variable is aligned wider than a pointer";
+ } else if (D->hasAttr<SectionAttr>()) {
+ if (EmitDiagnostic)
+ M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type)
+ << GVId << "of a section attribute";
+ } else if ((GV->hasExternalLinkage() || M.getCodeGenOpts().AllTocData) &&
+ !GV->hasInternalLinkage()) {
+ GVar->addAttribute("toc-data");
+ }
+ }
+ }
+}
+
// PowerPC-32
namespace {
/// PPC32_SVR4_ABIInfo - The 32-bit PowerPC ELF (SVR4) ABI information.
diff --git a/clang/lib/Driver/ToolChains/AIX.cpp b/clang/lib/Driver/ToolChains/AIX.cpp
index 3e5ebafa15ebe1c..47f4aca199a9c2b 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -421,6 +421,90 @@ void AIX::AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
llvm_unreachable("Unexpected C++ library type; only libc++ is supported.");
}
+// This function processes all the mtocdata options to build the final
+// simplified toc data options to pass to CC1.
+static void addTocDataOptions(const llvm::opt::ArgList &Args,
+ llvm::opt::ArgStringList &CC1Args,
+ const Driver &D) {
+
+ // Check the global toc-data setting. The default is -mno-tocdata.
+ // To enable toc-data globally, -mtocdata must be specified.
+ // Additionally, it must be last to take effect.
+ const bool TOCDataGloballyinEffect = [&Args]() {
+ if (!Args.hasArg(options::OPT_mtocdata))
+ return false;
+
+ const Arg *LastArg =
+ Args.getLastArg(options::OPT_mtocdata, options::OPT_mno_tocdata);
+ return LastArg->getOption().matches(options::OPT_mtocdata);
+ }();
+
+ // Currently only supported for small code model.
+ if (TOCDataGloballyinEffect &&
+ (Args.getLastArgValue(options::OPT_mcmodel_EQ).equals("large") ||
+ Args.getLastArgValue(options::OPT_mcmodel_EQ).equals("medium"))) {
+ D.Diag(clang::diag::warn_drv_unsupported_tocdata);
+ return;
+ }
+
+ enum TOCDataSetting {
+ AddressInTOC = 0, // Address of the symbol stored in the TOC.
+ DataInTOC = 1 // Symbol defined in the TOC.
+ };
+
+ const TOCDataSetting DefaultTocDataSetting =
+ TOCDataGloballyinEffect ? DataInTOC : AddressInTOC;
+
+ // Process the list of variables in the explicitly specified options
+ // -mtocdata= and -mno-tocdata= to see which variables are opposite to
+ // the global setting of tocdata in TOCDataGloballyinEffect.
+ // Those that have the opposite setting to TOCDataGloballyinEffect, are added
+ // to ExplicitlySpecifiedGlobals.
+ llvm::StringSet<> ExplicitlySpecifiedGlobals;
+ for (const auto Arg :
+ Args.filtered(options::OPT_mtocdata_EQ, options::OPT_mno_tocdata_EQ)) {
+ TOCDataSetting ArgTocDataSetting =
+ Arg->getOption().matches(options::OPT_mtocdata_EQ) ? DataInTOC
+ : AddressInTOC;
+
+ if (ArgTocDataSetting != DefaultTocDataSetting)
+ for (const char *Val : Arg->getValues())
+ ExplicitlySpecifiedGlobals.insert(Val);
+ else
+ for (const char *Val : Arg->getValues())
+ ExplicitlySpecifiedGlobals.erase(Val);
+ }
+
+ const bool HasExplicitValues = !ExplicitlySpecifiedGlobals.empty();
+
+ auto buildExceptionList = [](const llvm::StringSet<> &ExplicitValues,
+ const char *OptionSpelling) -> std::string {
+ std::string Option(OptionSpelling);
+ bool IsFirst = true;
+ for (const auto &E : ExplicitValues) {
+ if (!IsFirst)
+ Option += ",";
+
+ IsFirst = false;
+ Option += E.first();
+ }
+ return Option;
+ };
+
+ // Pass the final tocdata options to CC1 consisting of the default
+ // tocdata option (-mtocdata/-mno-tocdata) along with the list
+ // option (-mno-tocdata=/-mtocdata=) if there are any explicitly specified
+ // variables which would be exceptions to the default setting.
+ const char *TocDataGlobalOption =
+ TOCDataGloballyinEffect ? "-mtocdata" : "-mno-tocdata";
+ const char *TocDataListOption =
+ TOCDataGloballyinEffect ? "-mno-tocdata=" : "-mtocdata=";
+ CC1Args.push_back(TocDataGlobalOption);
+ if (HasExplicitValues)
+ CC1Args.push_back(Args.MakeArgString(llvm::Twine(
+ buildExceptionList(ExplicitlySpecifiedGlobals, TocDataListOption))));
+}
+
void AIX::addClangTargetOptions(
const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadingKind) const {
@@ -428,6 +512,11 @@ void AIX::addClangTargetOptions(
Args.AddLastArg(CC1Args, options::OPT_mdefault_visibility_export_mapping_EQ);
Args.addOptInFlag(CC1Args, options::OPT_mxcoff_roptr, options::OPT_mno_xcoff_roptr);
+ // Forward last mtocdata/mno_tocdata options to -cc1.
+ if (Args.hasArg(options::OPT_mtocdata_EQ, options::OPT_mno_tocdata_EQ,
+ options::OPT_mtocdata))
+ addTocDataOptions(Args, CC1Args, getDriver());
+
if (Args.hasFlag(options::OPT_fxl_pragma_pack,
options::OPT_fno_xl_pragma_pack, true))
CC1Args.push_back("-fxl-pragma-pack");
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index d18371f21a9d86e..3c606d78fc47c51 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1035,6 +1035,11 @@ bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
if (getFrontendOpts().ShowStats || !getFrontendOpts().StatsFile.empty())
llvm::EnableStatistics(false);
+ // Sort vectors containing toc data and no toc data variables to facilitate
+ // binary search later.
+ llvm::sort(getCodeGenOpts().TocDataVarsUserSpecified);
+ llvm::sort(getCodeGenOpts().NoTocDataVars);
+
for (const FrontendInputFile &FIF : getFrontendOpts().Inputs) {
// Reset the ID tables if we are reusing the SourceManager and parsing
// regular files.
diff --git a/clang/test/CodeGen/PowerPC/toc-data-attribute.c b/clang/test/CodeGen/PowerPC/toc-data-attribute.c
new file mode 100644
index 000000000000000..635ad53ce8c5d2d
--- /dev/null
+++ b/clang/test/CodeGen/PowerPC/toc-data-attribute.c
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 %s -triple powerpc-ibm-aix-xcoff -S -mtocdata=f,g,h,i,j,k,l,m,n,o,p -emit-llvm -o - 2>&1 | FileCheck %s -check-prefixes=CHECK32 --match-full-lines
+// RUN: %clang_cc1 %s -triple powerpc-ibm-aix-xcoff -S -mtocdata -emit-llvm -o - 2>&1 | FileCheck %s -check-prefixes=CHECK32 --match-full-lines
+
+// RUN: %clang_cc1 %s -triple powerpc64-ibm-aix-xcoff -S -mtocdata=f,g,h,i,j,k,l,m,n,o,p -emit-llvm -o - 2>&1 | FileCheck %s -check-prefixes=CHECK64 --match-full-lines
+// RUN: %clang_cc1 %s -triple powerpc64-ibm-aix-xcoff -S -mtocdata -emit-llvm -o - 2>&1 | FileCheck %s -check-prefixes=CHECK64 --match-full-lines
+
+extern int f;
+long long g = 5;
+const char *h = "h";
+int *i;
+int __attribute__((aligned(128))) j = 0;
+float k = 100.00;
+double l = 2.5;
+int m __attribute__((section("foo"))) = 10;
+__thread int n;
+
+extern int p[];
+
+struct SomeStruct;
+extern struct SomeStruct o;
+
+static int func_a() {
+ return g+(int)h[0]+*i+j+k+l+m+n+p[0];
+}
+
+int func_b() {
+ f = 1;
+ return func_a();
+}
+
+struct SomeStruct* getAddress(void) {
+ return &o;
+}
+
+// CHECK32: @g = global i64 5, align 8
+// CHECK32: {{.*}} = private unnamed_addr constant [2 x i8] c"h\00", align 1
+// CHECK32: @h = global {{...*}} #0
+// CHECK32: @j = global i32 0, align 128
+// CHECK32: @k = global float 1.000000e+02, align 4 #0
+// CHECK32: @l = global double 2.500000e+00, align 8
+// CHECK32: @m = global i32 10, section "foo", align 4
+// CHECK32: @f = external global ...
[truncated]
|
MaskRay
reviewed
Oct 26, 2023
diggerlin
reviewed
Oct 31, 2023
There was a problem hiding this comment.
What are the ABI implications here? Do all translation units have to agree on whether a variable is "tocdata"? The manual change hints there's some sort of linker fixup, but it's not clear to me how that works.
Specifying the names of global variables on the command-line is a bit unusual; we normally prefer attributes. Do you need that for XL compat?
The manual change says "the TOC data transformation will be applied to [...] block-scope static variables". But later, you mention "internal linkage", which is a bit confusing when you're dealing with C++. Block-scope static variables don't have "linkage" in the sense defined in the C++ standard. Maybe worth trying to be more explicit here.
clang/lib/CodeGen/Targets/PPC.cpp
Outdated
| M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type) | ||
| << GVId << "of a section attribute"; | ||
| } else if ((GV->hasExternalLinkage() || M.getCodeGenOpts().AllTocData) && | ||
| !GV->hasInternalLinkage()) { |
There was a problem hiding this comment.
Maybe should be hasLocalLinkage()?
diggerlin
reviewed
Oct 31, 2023
diggerlin
reviewed
Oct 31, 2023
MaskRay
reviewed
Oct 31, 2023
diggerlin
reviewed
Nov 1, 2023
diggerlin
reviewed
Nov 1, 2023
diggerlin
reviewed
Nov 1, 2023
diggerlin
reviewed
Nov 1, 2023
diggerlin
reviewed
Nov 2, 2023
diggerlin
reviewed
Nov 2, 2023
clang/test/CodeGen/toc-data.c
Outdated
| @@ -0,0 +1,31 @@ | |||
| // RUN: %clang_cc1 %s -triple powerpc-unknown-aix -S -mtocdata=g1,g2,g3 -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-MIX --match-full-lines | |||
There was a problem hiding this comment.
I am not sure the purpose of the test case, I think all the test scenario have be covered by previous test cases.
There are no ABI implications. Using -mtocdata removes a level of indirection, but you can mix tocdata and non-tocdata references to the same variable. The -mtocdata option is intended for variables that are part of the current loadable object and not imported. If you specify --mtocdata for a symbol that is actually imported, the linker will generate extra code (called "fixup" code) to reference the variable. A warning is printed because fixup code is not great for performance |
diggerlin
reviewed
Nov 2, 2023
clang/lib/Driver/ToolChains/AIX.cpp
Outdated
|
|
||
| const char *TocDataListOption = | ||
| TOCDataGloballyinEffect ? "-mno-tocdata=" : "-mtocdata="; | ||
| if (const bool HasExplicitValues = !ExplicitlySpecifiedGlobals.empty()) |
There was a problem hiding this comment.
sorry that please change to
if (!ExplicitlySpecifiedGlobals.empty())
diggerlin
reviewed
Nov 3, 2023
diggerlin
reviewed
Nov 3, 2023
clang/lib/CodeGen/Targets/PPC.cpp
Outdated
| return; | ||
|
|
||
| auto *GVar = dyn_cast<llvm::GlobalVariable>(GV); | ||
| auto GVId = M.getMangledName(dyn_cast<NamedDecl>(D)); |
There was a problem hiding this comment.
the GV->getName() is already mangle name, I just curiosity, why not use GV->getName() directly ?
syzaara
force-pushed
the
syzaara/tocdata
branch
from
3aab683 to
f23461a
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
diggerlin
reviewed
Jan 5, 2024
clang/lib/CodeGen/Targets/PPC.cpp
Outdated
| reportUnsupportedWarning(EmitDiagnostic, | ||
| "variable is aligned wider than a pointer"); | ||
| } else if (D->hasAttr<SectionAttr>()) { | ||
| reportUnsupportedWarning(EmitDiagnostic, "of a section attribute"); |
There was a problem hiding this comment.
of a section attribute
-->
variable has a section attribute
clang/lib/CodeGen/Targets/PPC.cpp
Outdated
| } else if (D->hasAttr<SectionAttr>()) { | ||
| reportUnsupportedWarning(EmitDiagnostic, "of a section attribute"); | ||
| } else if ((GV->hasExternalLinkage() || M.getCodeGenOpts().AllTocData) && | ||
| !GV->hasLocalLinkage()) { |
There was a problem hiding this comment.
if GV->hasExternalLinkage() is true , the !GV->hasLocalLinkage() is true too.
((GV->hasExternalLinkage() || M.getCodeGenOpts().AllTocData) && !GV->hasLocalLinkage())
look weirds. suggest changing to
(GV->hasExternalLinkage() || (M.getCodeGenOpts().AllTocData && !GV->hasLocalLinkage())
| @@ -0,0 +1,69 @@ | |||
| // REQUIRES: powerpc-registered-target | |||
There was a problem hiding this comment.
do we need the // REQUIRES: powerpc-registered-target since it cross compile?
diggerlin
reviewed
Jan 8, 2024
| @@ -245,6 +245,30 @@ class PPCSubtarget : public PPCGenSubtargetInfo { | |||
| /// True if the GV will be accessed via an indirect symbol. | |||
| bool isGVIndirectSymbol(const GlobalValue *GV) const; | |||
|
|
|||
| bool tocDataChecks(unsigned PointerSize, const GlobalVariable *GV) const { | |||
There was a problem hiding this comment.
the function always return true.
suggestion change
bool tocDataChecks(unsigned PointerSize, const GlobalVariable *GV) const
--->
void tocDataChecks(unsigned PointerSize, const GlobalVariable *GV) const
There was a problem hiding this comment.
and it is not a good idea to put a big function in the header file
| @@ -2520,7 +2520,9 @@ void PPCAIXAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) { | |||
| // If the Global Variable has the toc-data attribute, it needs to be emitted | |||
| // when we emit the .toc section. | |||
| if (GV->hasAttribute("toc-data")) { | |||
| TOCDataGlobalVars.push_back(GV); | |||
| unsigned PointerSize = GV->getParent()->getDataLayout().getPointerSize(); | |||
There was a problem hiding this comment.
if we run
if (G.hasAttribute("toc-data")) (Subtarget->tocDataChecks(G.getParent()->getDataLayout().getPointerSize(), &G));
in the function PPCAIXAsmPrinter::doInitialization(Module &M),
we do not need to do tocDataChecks in the function static bool hasTocDataAttr(SDValue Val, unsigned PointerSize, const PPCSubtarget *Subtarget) and here
The function hasTocDataAttr is run for each PPCISD::TOC_ENTRY node, if a toc variable is referenced several time by the PPCISD::TOC_ENTRY in user application, the function tocDataChecks will be called several times for the same variable and the tocDataCheck is not a small function.
diggerlin
reviewed
Jan 19, 2024
| @@ -510,7 +510,8 @@ SDNode *PPCDAGToDAGISel::getGlobalBaseReg() { | |||
| } | |||
|
|
|||
| // Check if a SDValue has the toc-data attribute. | |||
| static bool hasTocDataAttr(SDValue Val, unsigned PointerSize) { | |||
| static bool hasTocDataAttr(SDValue Val, unsigned PointerSize, | |||
| const PPCSubtarget *Subtarget) { | |||
There was a problem hiding this comment.
I do not think we still need the function parameter const PPCSubtarget *Subtarget ?
diggerlin
approved these changes
Jan 23, 2024
AaronHLiu
reviewed
Jan 30, 2024
| BothFlags<[], [ClangOption, CLOption]>>, Group<m_Group>; | ||
| def mtocdata_EQ : CommaJoined<["-"], "mtocdata=">, | ||
| Visibility<[ClangOption, CC1Option]>, | ||
| Flags<[TargetSpecific]>, |
There was a problem hiding this comment.
Need to remove all Flags<[TargetSpecific]>, otherwise will get errors in link step as follows:
error: unsupported option '-mtocdata' for target 'powerpc-ibm-aix7.2.0.0'
| MarshallingInfoStringVector<CodeGenOpts<"TocDataVarsUserSpecified">>; | ||
| def mno_tocdata_EQ : CommaJoined<["-"], "mno-tocdata=">, | ||
| Visibility<[ClangOption, CC1Option]>, | ||
| Flags<[TargetSpecific]>, |
There was a problem hiding this comment.
Need to remove all Flags<[TargetSpecific]>, otherwise will get errors in link step as follows:
error: unsupported option '-mtocdata' for target 'powerpc-ibm-aix7.2.0.0'
| MarshallingInfoStringVector<CodeGenOpts<"TocDataVarsUserSpecified">>; | ||
| def mno_tocdata_EQ : CommaJoined<["-"], "mno-tocdata=">, | ||
| Visibility<[ClangOption, CC1Option]>, | ||
| Flags<[TargetSpecific]>, |
There was a problem hiding this comment.
Need to remove all Flags<[TargetSpecific]>, otherwise will get errors in link step as follows:
error: unsupported option '-mtocdata' for target 'powerpc-ibm-aix7.2.0.0'
|
Ping. |
diggerlin
reviewed
Feb 12, 2024
| "currently supported by the toc data transformation."); | ||
| assert(!GV->hasCommonLinkage() && | ||
| "Tentative definitions cannot have the mapping class XMC_TD."); | ||
| } |
There was a problem hiding this comment.
since the https://github.com/llvm/llvm-project/pull/79530/files merge , please remember to remove
assert(!GV->hasCommonLinkage() &&
"Tentative definitions cannot have the mapping class XMC_TD.");
amy-kwan
approved these changes
Feb 16, 2024
clang/lib/CodeGen/Targets/PPC.cpp
Outdated
| M.getDiags().Report(D->getLocation(), diag::warn_toc_unsupported_type) | ||
| << GVId << Msg; | ||
| }; | ||
| if (!Ty || Ty->isIncompleteType()) { |
There was a problem hiding this comment.
nit: Since there's only one statement inside these conditions, I think we can omit the curly braces.
llvm/lib/MC/MCSectionXCOFF.cpp
Outdated
| @@ -82,8 +82,7 @@ void MCSectionXCOFF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T, | |||
| } | |||
|
|
|||
| if (isCsect() && getMappingClass() == XCOFF::XMC_TD) { | |||
| assert((getKind().isBSSExtern() || getKind().isBSSLocal()) && | |||
| "Unexepected section kind for toc-data"); | |||
| assert(getKind().isBSS() && "Unexepected section kind for toc-data"); | |||
There was a problem hiding this comment.
Suggested change
| assert(getKind().isBSS() && "Unexepected section kind for toc-data"); | |
| assert(getKind().isBSS() && "Unexpected section kind for toc-data"); |
syzaara
force-pushed
the
syzaara/tocdata
branch
from
f640393 to
f54af62
Compare
added 9 commits
March 12, 2024 12:28
This patch enables support that the XL compiler had for AIX under -qdatalocal/-qdataimported.
syzaara
force-pushed
the
syzaara/tocdata
branch
from
f54af62 to
c95a9cf
Compare
MaskRay
added a commit
that referenced
this pull request
Mar 13, 2024
MaskRay
reviewed
Mar 13, 2024
| std::vector<std::string> NoTocDataVars; | ||
|
|
||
| /// Flag for all global variables to be treated as toc-data. | ||
| bool AllTocData; |
There was a problem hiding this comment.
The uninitialized AllTocData would cause memorysanitizer failures.
booleans and enums should be moved to CodeGenOptionsBase using clang/Basic/CodeGenOptions.def.
I fixed this in 605abe0
|
This is broken by the patch https://lab.llvm.org/buildbot/#/builders/5/builds/41773/steps/9/logs/stdio |
Should be fixed by 00ba2a6 |
|
I'm seeing test failures locally when compiling with If you notice, the arg ordering is reversed ( |
Thanks, this should be fixed with #86840 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch enables support that the XL compiler had for AIX under -qdatalocal/-qdataimported.