[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked 4 inline comments as done.
stevewan added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+                                  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/AIX.h:26
+
+  bool hasIntegratedCPP() const override { return false; }
+
----------------
daltenty wrote:
> stevewan wrote:
> > Xiangling_L wrote:
> > > 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?
> > 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. 
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69620