Add AIX assembler support

[stevewan]

Summary

A skeleton of AIX toolchain and system linker support has been introduced in D68340, and this is a follow on patch to it.
This patch adds support to system assembler invocation to the AIX toolchain.

[Xiangling_L]

clang/lib/Driver/ToolChains/AIX.cpp

28

The definition of claimNoWarnArgs is to suppress warnings for some options if they are unused, can you explain a little bit about how did you figure out that we don't want warnings for those?

Some context of claimNoWarnArgs:

// Claim options we don't want to warn if they are unused. We do this for
// options that build systems might add but are unused when assembling or only
// running the preprocessor for example.
void tools::claimNoWarnArgs(const ArgList &Args) {
  // Don't warn about unused -f(no-)?lto.  This can happen when we're
  // preprocessing, precompiling or assembling.
  Args.ClaimAllArgs(options::OPT_flto_EQ);
  Args.ClaimAllArgs(options::OPT_flto);
  Args.ClaimAllArgs(options::OPT_fno_lto);
}

45

GCC invokes system assembler also with options -mpwr4 and -u, I think you need to verify that do we need those? And as far as I can recall, -mpwr4 is to pick up new version AIX instruction set, and -u is to suppress warning for undefined symbols. 90% sure that we need -mpwr4(I could be wrong), but not sure about -u.

clang/lib/Driver/ToolChains/AIX.h

26

I saw a lot of other target also set hasIntegratedCPP() as false, but I didn't find any explanation in documentation, so I am curious that what this is about?

[stevewan]

clang/lib/Driver/ToolChains/AIX.h

26

I also failed to find useful resources that explains the purpose of this function. The main rationales of adding it were essentially that,

1. It's a pure virtual function in the base Tool class,
2. Most if not all of other targets had overridden this function such that it returns false.

I'll leave this comment open, and see if someone could enlighten us.

[daltenty]

clang/lib/Driver/ToolChains/AIX.h

26

Only "Compiler" tools set hasIntegratedCPP() to true. Looking into it, it seems combineWithPreprocessor() uses this to decide whether the tool supports preprocessor actions so it may fold them in to one step. I think we are safe leaving this as is.

[stevewan]

clang/lib/Driver/ToolChains/AIX.cpp

45

For the -u option, I'll be adding it to this patch with a FIXME that we'd like to remove it in the future. The rationale is that, since currently we do not have the assembly writing ready that writes the extern(s), we'll pass in this -u to suppress any related warnings. Once the assembly writing is ready, we should remove this flag as it potentially covers up for undefined symbols that are actually problematic.

For the -m option, adding -many seemed to be more suitable at the moment. During code generation, the compiler should've already guaranteed that it used a correct and appropriate instruction set (i.e., conforms to the user specified driver option -mcpu=). The -m assembler option here double checks the same thing that's already been checked at codegen. That said, we can just pass in -many to accept all and rely on codegen to do the checking. On the other hand, potential problems might get away with it in one of the two scenarios I can think of right now,

1. User passes in a .s assembly file that uses instructions from the super-set of what's specified in -mcpu=. This is mostly on the user side because essentially they pass in contradictory inputs that are not going to fly.
2. User passes in a .c source file, but codegen hit some issue and falsely decides to use a super-set of what's specified in -mcpu=. Given that we don't have codegen ready yet, we don't know how reliable it might be. I would suggest that we keep -many as it is for now, and we may change it when needed once we are more clear on code generation.

With regard to GCC's behaviour, GCC would append first -mpwr4 then -many to the system assembler, which effectively means adding -many solely because the last -m flag would overwrite all its preceding one(s).

[stevewan]

clang/lib/Driver/ToolChains/AIX.cpp

28

The original reason was that, since we are doing assembling here, and as stated in claimNoWarnArgs, there might be an unused -f(no-)?lto when we're assembling, and we'd like to suppress the warning(s) for that.

Looking into it, the three options [[ https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-flto | flto, fno_lto ]] , and [[ https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-flto | flto_EQ ]] are used to enable/disable link time optimization, but LTO is not supported by the AIX system linker. Driver would just throw error if the user passes it -flto or -flto=<arg> on AIX, so claiming them or not currently makes no actual difference as far as I'm concerned.

That being said, I don't have a strong opinion either way. Let's see how other people think.

clang/lib/Driver/ToolChains/AIX.h

26

I checked this function on other targets. As David pointed out, only the compiler tools would set hasIntegratedCPP() to true, the assembler and linker tools set it to false because they do not support combining with preprocessor action.

[stevewan]

Add "-u" to accept undefined symbol as extern.

[stevewan]

clang/lib/Driver/ToolChains/AIX.cpp

28

It's probably also worth mentioning that, adding claimNoWarnArgs does not affect those LTO options being parsed and consumed by the driver in Driver::setLTOMode.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/AIX.cpp

37

Refer to the other comment regarding confusion of "as" with the English word.

47

There is no FIXME here indicating the plan to remove this in the future when the assembly generation from the compiler no longer needs this.

49

Minor nit: Typo: s/Acccept/Accept/;

50

A comment should indicate that this behaviour matches that of GCC on Power for AIX and Linux for both the user-provided assembler source case and the compiler-produced assembler source case. The behaviour for XL with user-provided assemble source is different.

61

The AIX assembler does not accept more than one input file. If the intent is to let the assembler produce the error message, then I think we should at least have a comment to acknowledge the situation.

clang/test/Driver/aix-as.c

1

Minor nit: To avoid confusion with the English word, use

`as`

or

as(1)

31

I am not getting how this block is related to "pthread".

40

s/powerpc/powerpc64/;

44

Same comment re: "pthread".

[stevewan]

Remove claimNoWarnArgs, limit the number of input files, and add more comments.

[stevewan]

clang/lib/Driver/ToolChains/AIX.cpp

28

Turned out Driver::BuildCompilation would always call Driver::setLTOMode, which inherently sets the three LTO options to "claimed". This happens before the driver gets to assembler invocation, hence no need to call claimNoWarnArgs here.

For future reference, I'm also documenting here that claimNoWarnArgs was added in rL225100, which was originally posted to fix build bots that failed due to warnings from unused LTO options. setLTOMode was introduced months later in rL250455, which claims the three LTO options during BuildCompilation.

61

Good point. I didn't intend to leave this to assembler. Looking into it, when presented multiple assembly sources, the driver would invoke as to assemble each of the input files individually. This behaviour also matches that of GCC and XL. In case something goes wrong and the driver attempts to pass multiple input files to as, adding a check to guard the number of inputs probably makes sense?

clang/test/Driver/aix-as.c

31

Looks like I copy-pasted the wrong thing while rebasing. Good catch, thanks!

[daltenty]

Other than minor nit, LGTM

clang/lib/Driver/ToolChains/AIX.cpp

68

nit: plural, "files"

[stevewan]

Correct a typo in a comment.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/AIX.cpp

68

llvm_unreachable is not the right solution if this can be reached by "user error". We should produce a proper error message.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/AIX.cpp

70

Should is also be checked as being either a filename or "nothing"?

[stevewan]

Add more input file handling

1. Check if the input file is of type Filename or Nothing, throw assertion otherwise.
2. Add a test case to verify the correct behaviour of assembler invocation when multiple input files are specified by the user.

[stevewan]

clang/lib/Driver/ToolChains/AIX.cpp

68

One risky condition I can think of is the user passes in multiple assembly sources to the driver, which may lead to multiple assembler inputs. To verify how the driver handles such a case, I added a new test into aix-as.c below, whose results suggested that this is okay because the driver would invoke as for each and every input files respectively. Looking into the code, the driver would construct an action list for each input files individually, which again matches the behaviour we observed in the testing results. That said, I believe the llvm_unreachable here is indeed not reachable by "user errors" like this, and if it triggers, it's likly an internal error.

[hubert.reinterpretcast]

LGTM with minor change request to a comment. Thanks.

clang/lib/Driver/ToolChains/AIX.cpp

68

Thanks. Can we indicate that we are expecting the driver to invoke as separately for each assembler source file in the comment?

[stevewan]

State in the comment the expectation to driver in handling assembler source input when invoking as(1).

clang/lib/Driver/ToolChains/AIX.h

 
 #include "clang/Driver/Tool.h"
 #include "clang/Driver/ToolChain.h"
 
 namespace clang {
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain &TC) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation &C, const JobAction &JA,
+                    const InputInfo &Output, const InputInfoList &Inputs,
+                    const llvm::opt::ArgList &TCArgs,
+                    const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain &TC) : Tool("aix::Linker", "linker", TC) {}
 
   bool hasIntegratedCPP() const override { return false; }
   bool isLinkJob() const override { return true; }
 
   void ConstructJob(Compilation &C, const JobAction &JA,
public:
   AIX(const Driver &D, const llvm::Triple &Triple,
       const llvm::opt::ArgList &Args);
 
   bool isPICDefault() const override { return true; }
   bool isPIEDefault() const override { return false; }
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_AIX_H

clang/lib/Driver/ToolChains/AIX.cpp

 #include "llvm/Option/ArgList.h"
 
 using AIX = clang::driver::toolchains::AIX;
 using namespace clang::driver;
 using namespace clang::driver::tools;
 
 using namespace llvm::opt;
 
+void aix::Assembler::ConstructJob(Compilation &C, const JobAction &JA,
+                                  const InputInfo &Output,
+                                  const InputInfoList &Inputs,
+                                  const ArgList &Args,
+                                  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;
+
+  const bool IsArch32Bit = getToolChain().getTriple().isArch32Bit();
+  const bool IsArch64Bit = getToolChain().getTriple().isArch64Bit();
+  // Only support 32 and 64 bit.
+  if (!IsArch32Bit && !IsArch64Bit)
+    llvm_unreachable("Unsupported bit width value.");
+
+  // Specify the mode in which the as command operates.
+  if (IsArch32Bit) {
+    CmdArgs.push_back("-a32");
+  } else {
+    // Must be 64-bit, otherwise asserted already.
+    CmdArgs.push_back("-a64");
+  }
+
+  // Accepts an undefined symbol as an extern so that an error message is not
+  // displayed. Otherwise, undefined symbols are flagged with error messages.
+  CmdArgs.push_back("-u");
+
+  // Acccept any mixture of instructions.
+  CmdArgs.push_back("-many");
+
+  Args.AddAllArgValues(CmdArgs, options::OPT_Wa_COMMA, options::OPT_Xassembler);
+
+  // Specify assembler output file.
+  assert((Output.isFilename() || Output.isNothing()) && "Invalid output.");
+  if (Output.isFilename()) {
+    CmdArgs.push_back("-o");
+    CmdArgs.push_back(Output.getFilename());
+  }
+
+  // Specify assembler input file(s).
+  for (const auto &II : Inputs)
+    CmdArgs.push_back(II.getFilename());
+
+  const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("as"));
+  C.addCommand(std::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
+}
+
 void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                const InputInfo &Output,
                                const InputInfoList &Inputs, const ArgList &Args,
                                const char *LinkingOutput) const {
   const AIX &ToolChain = static_cast<const AIX &>(getToolChain());
   ArgStringList CmdArgs;
 
   const bool IsArch32Bit = ToolChain.getTriple().isArch32Bit();
   const bool IsArch64Bit = ToolChain.getTriple().isArch64Bit();
   // Only support 32 and 64 bit.
   if (!(IsArch32Bit || IsArch64Bit))
     llvm_unreachable("Unsupported bit width value.");
 
   // Force static linking when "-static" is present.
   if (Args.hasArg(options::OPT_static))
     CmdArgs.push_back("-bnso");
 
   // Specify linker output file.
   assert((Output.isFilename() || Output.isNothing()) && "Invalid output.");
   if (Output.isFilename()) {
     CmdArgs.push_back("-o");
     CmdArgs.push_back(Output.getFilename());
-  } 
+  }
 
   // Set linking mode (i.e., 32/64-bit) and the address of
   // text and data sections based on arch bit width.
   if (IsArch32Bit) {
     CmdArgs.push_back("-b32");
     CmdArgs.push_back("-bpT:0x10000000");
     CmdArgs.push_back("-bpD:0x20000000");
   } else {
if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
 
     CmdArgs.push_back("-lc");
   }
 
   const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath());
   C.addCommand(std::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
 }
 
-/// AIX - AIX tool chain which can call ld(1) directly.
-// TODO: Enable direct call to as(1).
+/// AIX - AIX tool chain which can call as(1) and ld(1) directly.
 AIX::AIX(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
     : ToolChain(D, Triple, Args) {
   getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
 }
 
+auto AIX::buildAssembler() const -> Tool * { return new aix::Assembler(*this); }
+
 auto AIX::buildLinker() const -> Tool * { return new aix::Linker(*this); }

clang/test/Driver/aix-as.c

+// General tests that as invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN:         -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN:         -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler <arg> option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN:         -Xassembler -w \
+// RUN:         -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-PTHREAD %s
+// CHECK-AS32-PTHREAD-NOT: warning:
+// CHECK-AS32-PTHREAD: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-PTHREAD: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-PTHREAD: "-a32" 
+// CHECK-AS32-PTHREAD: "-u" 
+// CHECK-AS32-PTHREAD: "-many"
+// CHECK-AS32-PTHREAD: "-w"
+
+// Check powerpc-ibm-aix7.1.0.0, 64-bit. -Wa,<arg>,<arg> option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN:         -Wa,-v,-w \
+// RUN:         -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-PTHREAD %s
+// CHECK-AS64-PTHREAD-NOT: warning:
+// CHECK-AS64-PTHREAD: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-PTHREAD: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-PTHREAD: "-a64" 
+// CHECK-AS64-PTHREAD: "-u" 
+// CHECK-AS64-PTHREAD: "-many"
+// CHECK-AS64-PTHREAD: "-v"
+// CHECK-AS64-PTHREAD: "-w"