[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked an inline comment as done.
stevewan added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+                                  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;
----------------
stevewan wrote:
> stevewan wrote:
> > Xiangling_L wrote:
> > > 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);
> > > }
> > > ```
> > > 
> > 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.
> 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`.
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 [[ https://reviews.llvm.org/rL225100 | rL225100 ]], which was originally posted to fix build bots that failed due to warnings from unused LTO options. `setLTOMode` was introduced months later in [[ https://reviews.llvm.org/rL250455 | rL250455 ]], which claims the three LTO options during `BuildCompilation`.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:61
+
+  // Specify assembler input file(s).
+  for (const auto &II : Inputs)
----------------
hubert.reinterpretcast wrote:
> 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.
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?


================
Comment at: clang/test/Driver/aix-as.c:31
+// RUN:         -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-PTHREAD %s
+// CHECK-AS32-PTHREAD-NOT: warning:
----------------
hubert.reinterpretcast wrote:
> I am not getting how this block is related to "pthread".
Looks like I copy-pasted the wrong thing while rebasing. Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620