[libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[qiongsiwu1]

Summary

libLTO currently ignores the -f[no-]integrated-as flags. This patch teaches libLTO to respect them on AIX.

The implementation consists of two parts:

1. Migrate llc's -no-integrated-as option to a codegen option so that the option is available to libLTO/lld/gold.
2. Teach clang to pass -no-integrated-as accordingly to libLTO depending on the -f[no-]integrated-as flags.

On platforms other than AIX, the -f[no-]integrated-as flags are ignored.

[steven_wu]

lld does not use LTOCodeGenerator.

I am also not sure how disable integrated assembler suppose to work? Do you have an end to end design for how to invoke assembler during linking? For current libLTO API, this is probably going to produce assembler file buffer to linker, which is not usable by linker.

[qiongsiwu1]

In D152924#4421624, @steven_wu wrote:

lld does not use LTOCodeGenerator.

I am also not sure how disable integrated assembler suppose to work? Do you have an end to end design for how to invoke assembler during linking? For current libLTO API, this is probably going to produce assembler file buffer to linker, which is not usable by linker.

Thanks for chiming in @steven_wu! This patch is enabling -f[no-]-integrated-as on AIX only. There is already code to invoke the assembler during linking https://github.com/llvm/llvm-project/blob/807adcf4b9cca5586fd1fb669811768528d9dd1d/llvm/lib/LTO/LTOCodeGenerator.cpp#L343. clang will emit a warning on other platforms. The patch title is revised to reflect that this feature is AIX only at the moment.

[steven_wu]

I see. Sounds fine to me.

[shchenz]

@qiongsiwu1 Hi, thanks for the patch. Almost good to me, except one small nit.

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

Seems other options leverage the default value in the back end, for example the default value for DisableIntegratedAS in backend is false, so when the front end requires integrated-as, maybe we can save the option here?

[qiongsiwu1]

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

Ah thanks for the comment!

maybe we can save the option here?

Could you help me understand what we mean by "the option"? Do we mean saving (or using?) the value of -f[no]-integrated-as explicitly here somehow instead of relying on ToolChain.useIntegratedAs()? How do we intend to use the saved option value? My understanding is that DisableIntegratedAS takes its value from the option -no-integrated-as if -no-integrated-as is specified. As pointed out eariler, DisableIntegratedAS is false by default. This code explicitly sets -no-integrated-as to 0 or 1, so for the LTO use case, we overwrite the back end default since the option is always present.

[shchenz]

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

For example, if front-end requires to use integrated-assembler which is same with back-end's default value, we don't need to pass -no-integrated-as=0? We only pass the option -no-integrated-as=1 when if (IsOSAIX && !UseIntegratedAs).

[qiongsiwu1]

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

Ah I see! Thanks for the clarification!

@w2yehia and I discussed this and we preferred setting the option explicitly regardless of the backend's default. The reason was that we did not want to leave a hanging case where the option was not passed to the backend, a case in which we would rely on a non-local option(DisableIntegratedAS)'s default value to control the backend. In other words, always passing in the option allowed us to reason about this code locally within this file. @w2yehia feel free to chime in if I am not describing our discussion correctly.

Could you help me understand the benefit of not passing in the option for the default case?

[shchenz]

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

Could you help me understand the benefit of not passing in the option for the default case?

Too many options pass from front-end to back-end is a reason. And another reason is: I met one case that there is a back-end option has no default value, so each front-end, like clang and FORTRAN will have to explicitly pass the same option. I was asked to set a default in the back-end, so no need to explicitly set the options in each front-end.

If what I read is right, some bool type options like EmulatedTLS, EnableStackSizeSection are only passed when their values are not the same with the back-end's default.

I am ok to keep it as now if you guys already have an agreement. This is just minor I think.

[shchenz]

Seems passing -Wl,-bplugin_opt:-no-integrated-as=1 from clang driver does not work as expected. Is this expected or by design(We can only use -fno-integrated-as but no -Wl,-bplugin_opt:-no-integrated-as=1)?

clang -flto -Wl,-bplugin_opt:-no-integrated-as=1 *.c -v
  -bplugin_opt:-no-integrated-as=1 -bplugin:../lib/libLTO.so -bplugin_opt:-mcpu=pwr7 -bplugin_opt:-no-integrated-as=0 ;;; Note the second -no-integrated-as=0 pass from clang front-end.

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

If you are going to add -no-integrated-as=0, I suggest that you rename the option to -integrated-as and use -integrated-as=1 instead.

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

701

The patch is titled AIX but this would change the Linux behavior. In my experience this diagnostic is not useful.

[qiongsiwu1]

Revising the option processing logic to not pass in -no-integrated-as=0. Thanks @shchenz and @MaskRay for the feedback!

[qiongsiwu1]

clang/lib/Driver/ToolChains/CommonArgs.cpp

698

-no-integrated-as=0 is no longer added. The backend default will take care of the case.

[qiongsiwu1]

In D152924#4426895, @shchenz wrote:

Seems passing -Wl,-bplugin_opt:-no-integrated-as=1 from clang driver does not work as expected. Is this expected or by design(We can only use -fno-integrated-as but no -Wl,-bplugin_opt:-no-integrated-as=1)?

clang -flto -Wl,-bplugin_opt:-no-integrated-as=1 *.c -v
  -bplugin_opt:-no-integrated-as=1 -bplugin:../lib/libLTO.so -bplugin_opt:-mcpu=pwr7 -bplugin_opt:-no-integrated-as=0 ;;; Note the second -no-integrated-as=0 pass from clang front-end.

Thanks for catching this! The latest patch fixed the problem. As you rightly pointed out, always passing in -no-integrated-as=0 by default interferes with uses such as above.

[qiongsiwu1]

clang/lib/Driver/ToolChains/CommonArgs.cpp

701

Yes I agree the title is a bit misleading. The warning is added because the behaviour of -fno-integrated-as changes with and without -flto. Without -flto, clang does create the job to invoke system assembler, and with -flto, currently clang silently ignores the -fno-integrated-as option (without this patch). I felt that was unexpected, hence I added the warning. I am happy to remove the warning if the community thinks that silently ignoring the option is acceptable.

The limitation of this diagnostic is that it goes through clang and there is nothing in the backend guarding against using -no-integrated-as directly through -plugin-opt on Linux, which will be ignored. @MaskRay is this something we should worry about? If not, I will not try looking for a place to add the warning in the backend.

Thanks!

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

701

I do not worry about LTO ignoring -fno-integrated-as on Linux/*BSD targets. The use cases are very limited and the primary user (Linux kernel community) knows this.

Actually the link action ignores compilation-affected options anyway (all CompileOnly_option).

[qiongsiwu1]

Address code review.

[qiongsiwu1]

clang/lib/Driver/ToolChains/CommonArgs.cpp

701

Thanks! The diagnostic is removed.

[qiongsiwu1]

Clean up code in addLTOOptions.

[qiongsiwu1]

@MaskRay @w2yehia @shchenz could you let me know if there are further comments? I will land this patch if there are none. Thanks!

[MaskRay]

clang/test/Driver/lto-aix.c

76

This ^//$ line is not useful. Some tests use this style, but it's not a good style.

If we remove this line, we can actually use Vim { } navigate among tests.

79

2-space indentation

llvm/test/tools/llvm-lto/aix-sys-as.ll

12 ↗

(On Diff #532256)

delete trailing blank line

[MaskRay]

We have TargetOptions::DisableIntegratedAS (your llc.cpp change). Do you know why it is not feasible for the places you want to check?

[qiongsiwu1]

In D152924#4490581, @MaskRay wrote:

We have TargetOptions::DisableIntegratedAS (your llc.cpp change). Do you know why it is not feasible for the places you want to check?

I am checking/using TargetOptions::DisableIntegratedAS in LTOCodeGenerator::useAIXSystemAssembler() to determine if the system assembler should be used. NoIntegratedAssembler is moved from an llc only option to a codegen option so LTO can use it as well.

[MaskRay]

Some nits about testing, otherwise LG

In D152924#4490950, @qiongsiwu1 wrote:

In D152924#4490581, @MaskRay wrote:

We have TargetOptions::DisableIntegratedAS (your llc.cpp change). Do you know why it is not feasible for the places you want to check?

I am checking/using TargetOptions::DisableIntegratedAS in LTOCodeGenerator::useAIXSystemAssembler() to determine if the system assembler should be used. NoIntegratedAssembler is moved from an llc only option to a codegen option so LTO can use it as well.

Sorry, my question was not clear. Anyway, I figured it out.
We need to pass lto::Config to lto::backend with the correct CGFileType information. TargetOptions::DisableIntegratedAS is already present, but we don't initialize it in llvm/lib/CodeGen/CommandFlags.cpp.
Thanks. This looks good to me.

llvm/test/tools/llvm-lto/aix-sys-as.ll

1 ↗

(On Diff #532256)

I suggest that you merge this test into aix.ll.

You can use

RUN: %if system-aix %{ ... %}

to only run a RUN line when the lit feature system-aix is available.

llvm/test/tools/llvm-lto/aix.ll

4

It seems that with or without -o, the behavior may be different?

In some lit configurations (e.g., Google's internal lit runner), the current working directory if a test invocation is read-only. We need to ensure that the directory is writable.

You can do

RUN: rm -rf %t && mkdir %t && cd %t`
RUN: llvm-as < %s -o a.bc
RUN: llvm-lto a.bc

[qiongsiwu1]

Address test case review comments. Thanks so much @MaskRay for the feedback!

clang/lib/Driver/ToolChains/CommonArgs.cpp

else if (A->getOption().matches(options::OPT_gdbx))
       CmdArgs.push_back(
           Args.MakeArgString(Twine(PluginOptPrefix) + "-debugger-tune=dbx"));
     else
       CmdArgs.push_back(
           Args.MakeArgString(Twine(PluginOptPrefix) + "-debugger-tune=gdb"));
   }
 
   if (IsOSAIX) {
+    if (!ToolChain.useIntegratedAs())
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine(PluginOptPrefix) + "-no-integrated-as=1"));
+
     // On AIX, clang assumes strict-dwarf is true if any debug option is
     // specified, unless it is told explicitly not to assume so.
     Arg *A = Args.getLastArg(options::OPT_g_Group);
     bool EnableDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
                            !A->getOption().matches(options::OPT_ggdb0);
     if (EnableDebugInfo && Args.hasFlag(options::OPT_gstrict_dwarf,
                                         options::OPT_gno_strict_dwarf, true))
       CmdArgs.push_back(
           Args.MakeArgString(Twine(PluginOptPrefix) + "-strict-dwarf=true"));
 
     for (const Arg *A : Args.filtered_reverse(options::OPT_mabi_EQ)) {

clang/test/Driver/lto-aix.c

 // Test LTO path, mcpu and opt level options
 // RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fuse-ld=ld -O3 2>&1 \
 // RUN:   | FileCheck -check-prefixes=LTOPATH,MCPUOPTLEVEL %s
 //
 // LTOPATH: "-bplugin:{{.*}}libLTO.{{so|dll|dylib}}"
 // MCPUOPTLEVEL: "-bplugin_opt:-mcpu={{.*}}" "-bplugin_opt:-O3"
-//
+
 // More opt level option tests
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -O -### 2>&1 | FileCheck --check-prefix=O1 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -O1 -### 2>&1 | FileCheck --check-prefix=O1 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -Og -### 2>&1 | FileCheck --check-prefix=O1 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -O2 -### 2>&1 | FileCheck --check-prefix=O2 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -Os -### 2>&1 | FileCheck --check-prefix=O2 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -Oz -### 2>&1 | FileCheck --check-prefix=O2 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -O3 -### 2>&1 | FileCheck --check-prefix=O3 %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -Ofast -### 2>&1 | FileCheck --check-prefix=O3 %s
 //
 // O1: "-bplugin_opt:-O1"
 // O2: "-bplugin_opt:-O2"
 // O3: "-bplugin_opt:-O3"
-//
+
 // vec-extabi option
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -mabi=vec-extabi -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=VECEXTABI %s
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -### 2>&1 | FileCheck --check-prefix=NOVECEXTABI %s
 //
 // VECEXTABI: "-bplugin_opt:-vec-extabi"
 // NOVECEXTABI-NOT: "-bplugin_opt:-vec-extabi"
-//
+
 // Test debugging options
 // RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fuse-ld=ld -gdbx 2>&1 \
 // RUN:   | FileCheck -check-prefix=DBX %s
 // RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fuse-ld=ld -g 2>&1 \
 // RUN:   | FileCheck -check-prefix=NODEBUGGER-TUNE %s
 // RUN: %clang --target=powerpc-ibm-aix-xcoff -### %s -flto -g 2>&1 \
 // RUN:   | FileCheck -check-prefixes=STRICT,NODEBUGGER-TUNE %s
 // RUN: %clang --target=powerpc64-ibm-aix-xcoff -### %s -flto -g 2>&1 \

 // RUN:   | FileCheck -check-prefix=NOSTRICT %s
 //
 // DBX:    "-bplugin_opt:-debugger-tune=dbx"
 // GDB:    "-bplugin_opt:-debugger-tune=gdb"
 // NODEBUGGER-TUNE-NOT: "-bplugin_opt:-debugger-tune="
 //
 // STRICT:       "-bplugin_opt:-strict-dwarf=true"
 // NOSTRICT-NOT: "-bplugin_opt:-strict-dwarf=true"
-//
+
 // Test cspgo options
 // RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fuse-ld=ld \
 // RUN:   -fcs-profile-generate 2>&1 | FileCheck -check-prefix=CSPGO %s
 //
 // CSPGO: "-bplugin_opt:-cs-profile-generate" "-bplugin_opt:-cs-profile-path=default_%m.profraw"
+
+// Test integrated assembler options
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fno-integrated-as \
+// RUN:   -fintegrated-as 2>&1 | FileCheck --check-prefix=INTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fintegrated-as \
+// RUN:   -fno-integrated-as 2>&1 | FileCheck --check-prefix=NOINTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto 2>&1 \
+// RUN:   | FileCheck --check-prefix=INTAS %s
+//
+// NOINTAS: "-bplugin_opt:-no-integrated-as=1"
+// INTAS-NOT: "-bplugin_opt:-no-integrated-as"

llvm/include/llvm/CodeGen/CommandFlags.h

 unsigned getOverrideStackAlignment();
 
 bool getStackRealign();
 
 std::string getTrapFuncName();
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool getRelaxELFRelocations();
 
 bool getDataSections();
 std::optional<bool> getExplicitDataSections();
 
 bool getFunctionSections();
 std::optional<bool> getExplicitFunctionSections();
 

llvm/lib/CodeGen/CommandFlags.cpp

 CGOPT(SwiftAsyncFramePointerMode, SwiftAsyncFramePointer)
 CGOPT(bool, DontPlaceZerosInBSS)
 CGOPT(bool, EnableGuaranteedTailCallOpt)
 CGOPT(bool, DisableTailCalls)
 CGOPT(bool, StackSymbolOrdering)
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
 CGOPT(bool, IgnoreXCOFFVisibility)
 CGOPT(bool, XCOFFTracebackTable)
 CGOPT(std::string, BBSections)
 CGOPT(unsigned, TLSSize)
 CGOPT_EXP(bool, EmulatedTLS)
#define CGBINDOPT(NAME)                                                        \
 
   static cl::opt<bool> XCOFFReadOnlyPointers(
       "mxcoff-roptr",
       cl::desc("When set to true, const objects with relocatable address "
                "values are put into the RO data section."),
       cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt<bool> DisableIntegratedAS(
+      "no-integrated-as", cl::desc("Disable integrated assembler"),
+      cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
 }
 
 llvm::BasicBlockSection
 codegen::getBBSectionsMode(llvm::TargetOptions &Options) {
   if (getBBSections() == "all")
Options.HonorSignDependentRoundingFPMathOption =
       getEnableHonorSignDependentRoundingFPMath();
   if (getFloatABIForCalls() != FloatABI::Default)
     Options.FloatABIType = getFloatABIForCalls();
   Options.EnableAIXExtendedAltivecABI = getEnableAIXExtendedAltivecABI();
   Options.NoZerosInBSS = getDontPlaceZerosInBSS();
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
       getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
   Options.FunctionSections = getFunctionSections();
   Options.IgnoreXCOFFVisibility = getIgnoreXCOFFVisibility();
   Options.XCOFFTracebackTable = getXCOFFTracebackTable();
   Options.BBSections = getBBSectionsMode(Options);
   Options.UniqueSectionNames = getUniqueSectionNames();

llvm/lib/LTO/LTOCodeGenerator.cpp

bool LTOCodeGenerator::writeMergedModules(StringRef Path) {
   }
 
   Out.keep();
   return true;
 }
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto &Triple = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> &AssemblyFile) {
   assert(useAIXSystemAssembler() &&
          "Runing AIX system assembler when integrated assembler is available!");
 
   // Set the system assembler path.
   SmallString<256> AssemblerPath("/usr/bin/as");

llvm/test/tools/llvm-lto/aix.ll

-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
+; RUN: rm -rf %t && mkdir %t && cd %t
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
+; Test system assembler.
+; RUN: %if system-aix %{ llvm-lto -no-integrated-as=1 %t1 | FileCheck %s %}
+
 target triple = "powerpc-ibm-aix"
 
 define i32 @main() {
 entry:
   ret i32 42
 }
 ; CHECK: Wrote native object file
-

llvm/tools/llc/llc.cpp

BinutilsVersion("binutils-version", cl::Hidden,
                     cl::desc("Produced object files can use all ELF features "
                              "supported by this binutils version and newer."
                              "If -no-integrated-as is specified, the generated "
                              "assembly will consider GNU as support."
                              "'none' means that all ELF features can be used, "
                              "regardless of binutils support"));
 
 static cl::opt<bool>
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-                      cl::desc("Disable integrated assembler"));
-
-static cl::opt<bool>
     PreserveComments("preserve-as-comments", cl::Hidden,
                      cl::desc("Preserve Comments in outputted assembly"),
                      cl::init(true));
 
 // Determine optimization level.
 static cl::opt<char>
     OptLevel("O",
              cl::desc("Optimization level. [-O0, -O1, -O2, or -O3] "
if (Options.XCOFFReadOnlyPointers) {
       // surprise of not respecting the setting.
       if (!Options.DataSections)
         reportError("-mxcoff-roptr option must be used with -data-sections",
                     InputFilename);
     }
 
     Options.BinutilsVersion =
         TargetMachine::parseBinutilsVersion(BinutilsVersion);
-    Options.DisableIntegratedAS = NoIntegratedAssembler;
     Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
     Options.MCOptions.AsmVerbose = AsmVerbose;
     Options.MCOptions.PreserveAsmComments = PreserveComments;
     Options.MCOptions.IASSearchPaths = IncludeDirs;
     Options.MCOptions.SplitDwarfFile = SplitDwarfFile;
     if (DwarfDirectory.getPosition()) {
       Options.MCOptions.MCUseDwarfDirectory =
           DwarfDirectory ? MCTargetOptions::EnableDwarfDirectory